-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update sql-formatter & specify SQL language according to warehouse #1490
Changes from 8 commits
c1fde96
6c40d57
2d1e1fa
e74f341
99f6d68
70338e0
9a61314
96ee5c2
11f9b2f
e92eeca
8c9093c
c6c296e
9d797db
240ae45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,10 @@ export enum WarehouseType { | |
SQLDATAWAREHOUSE = "sqldatawarehouse" | ||
} | ||
|
||
export function isWarehouseType(input: any): input is WarehouseType { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this does type narrowing using user-defined type guard, it's fine to take a string, but I think it would be better to take any or unknown. |
||
return Object.values(WarehouseType).includes(input); | ||
} | ||
|
||
const CANCELLATION_SUPPORTED = [WarehouseType.BIGQUERY, WarehouseType.SQLDATAWAREHOUSE]; | ||
|
||
export function supportsCancel(warehouseType: WarehouseType) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import * as jsBeautify from "js-beautify"; | |
import * as sqlFormatter from "sql-formatter"; | ||
import { promisify } from "util"; | ||
|
||
import { WarehouseType } from "df/core/adapters"; | ||
import { ErrorWithCause } from "df/common/errors/errors"; | ||
import { SyntaxTreeNode, SyntaxTreeNodeType } from "df/sqlx/lexer"; | ||
import { v4 as uuidv4 } from "uuid"; | ||
|
@@ -15,13 +16,26 @@ const JS_BEAUTIFY_OPTIONS: JsBeautifyOptions = { | |
|
||
const MAX_SQL_FORMAT_ATTEMPTS = 5; | ||
|
||
export type SqlLanguage = sqlFormatter.SqlLanguage; | ||
const WAREHOUSE_LANGUAGE_MAP: Record<WarehouseType, sqlFormatter.SqlLanguage> = { | ||
[WarehouseType.BIGQUERY]:"bigquery", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: seems unformatted |
||
[WarehouseType.PRESTO]: "trino", | ||
[WarehouseType.POSTGRES]: "postgresql", | ||
[WarehouseType.REDSHIFT]: "redshift", | ||
[WarehouseType.SNOWFLAKE]: "snowflake", | ||
[WarehouseType.SQLDATAWAREHOUSE]: "transactsql" | ||
}; | ||
|
||
const DEFAULT_WAREHOUSE_FOR_FORMATTING: WarehouseType = WarehouseType.BIGQUERY; | ||
|
||
export function format(text: string, fileExtension: string, language: SqlLanguage) { | ||
export function format( | ||
text: string, | ||
fileExtension: string, | ||
warehouse: WarehouseType = DEFAULT_WAREHOUSE_FOR_FORMATTING | ||
) { | ||
try { | ||
switch (fileExtension) { | ||
case "sqlx": | ||
return postProcessFormattedSqlx(formatSqlx(SyntaxTreeNode.create(text), "", language)); | ||
return postProcessFormattedSqlx(formatSqlx(SyntaxTreeNode.create(text), "", warehouse)); | ||
case "js": | ||
return `${formatJavaScript(text).trim()}\n`; | ||
default: | ||
|
@@ -36,15 +50,14 @@ export async function formatFile( | |
filename: string, | ||
options?: { | ||
overwriteFile?: boolean; | ||
language?: SqlLanguage; | ||
warehouse?: WarehouseType; | ||
} | ||
) { | ||
const fileExtension = filename.split(".").slice(-1)[0]; | ||
const originalFileContent = await promisify(fs.readFile)(filename, "utf8"); | ||
|
||
const language = options?.language || 'bigquery'; | ||
const formattedText = format(originalFileContent, fileExtension, language); | ||
if (formattedText !== format(formattedText, fileExtension, language)) { | ||
const formattedText = format(originalFileContent, fileExtension, options?.warehouse); | ||
if (formattedText !== format(formattedText, fileExtension, options?.warehouse)) { | ||
throw new Error("Formatter unable to determine final formatted form."); | ||
} | ||
|
||
|
@@ -61,10 +74,11 @@ export async function formatFile( | |
return formattedText; | ||
} | ||
|
||
function formatSqlx(node: SyntaxTreeNode, indent: string = "", language: SqlLanguage) { | ||
function formatSqlx(node: SyntaxTreeNode, indent: string = "", warehouse: WarehouseType) { | ||
const { sqlxStatements, javascriptBlocks, innerSqlBlocks } = separateSqlxIntoParts( | ||
node.children() | ||
); | ||
const sqlLanguage = WAREHOUSE_LANGUAGE_MAP[warehouse]; | ||
|
||
// First, format the JS blocks (including the config block). | ||
const formattedJsCodeBlocks = javascriptBlocks.map(jsCodeBlock => | ||
|
@@ -77,7 +91,7 @@ function formatSqlx(node: SyntaxTreeNode, indent: string = "", language: SqlLang | |
[placeholderId: string]: SyntaxTreeNode | string; | ||
} = {}; | ||
const unformattedPlaceholderSql = stripUnformattableText(sqlxStatement, placeholders).join(""); | ||
const formattedPlaceholderSql = formatSql(unformattedPlaceholderSql, language); | ||
const formattedPlaceholderSql = formatSql(unformattedPlaceholderSql, sqlLanguage); | ||
return formatEveryLine( | ||
replacePlaceholders(formattedPlaceholderSql, placeholders), | ||
line => `${indent}${line}` | ||
|
@@ -105,7 +119,7 @@ function formatSqlx(node: SyntaxTreeNode, indent: string = "", language: SqlLang | |
]); | ||
|
||
return `${upToFirstBrace} | ||
${formatSqlx(sqlCodeBlockWithoutOuterBraces, " ", language)} | ||
${formatSqlx(sqlCodeBlockWithoutOuterBraces, " ", warehouse)} | ||
${lastBraceOnwards}`; | ||
}); | ||
|
||
|
@@ -205,7 +219,7 @@ function formatJavaScript(text: string) { | |
return jsBeautify.js(text, JS_BEAUTIFY_OPTIONS); | ||
} | ||
|
||
function formatSql(text: string, language: SqlLanguage) { | ||
function formatSql(text: string, language: sqlFormatter.SqlLanguage) { | ||
let formatted = sqlFormatter.format(text, { language }) as string; | ||
// Unfortunately sql-formatter does not always produce final formatted output (even on plain SQL) in a single pass. | ||
for (let attempts = 0; attempts < MAX_SQL_FORMAT_ATTEMPTS; attempts++) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
# NOTE: If you change the format of this line, you must change the bash command | ||
# in /scripts/publish to extract the version string correctly. | ||
DF_VERSION = "2.5.0" | ||
DF_VERSION = "2.6.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined a immediate function because it is convenient for typing and scoping