[SQL] Compiler option --format to pretty-print SQL#6563
Conversation
|
I will need to merge apache/calcite#5057 first |
|
I merged it, and advanced the Calcite version too to include that change |
|
There is no manager API to request a reformat from the compiler server, this will be a separate issue |
mythical-fred
left a comment
There was a problem hiding this comment.
Nice work — solid feature with a real round-trip test (testReformat re-parses the reformatted output with --noRust). A few small things to consider; none blocking.
| * @param saveLines True if the lines are from the user program; false if they are internally generated. */ | ||
| public List<ParsedStatement> parseStatements(String statements, boolean saveLines) throws SqlParseException { | ||
| // Debugging flag for (partially) testing SqlPrettyPrinter | ||
| final boolean ROUND_TRIP_CHECK = false; |
There was a problem hiding this comment.
nit: committing a final boolean ROUND_TRIP_CHECK = false; makes this self-test permanently dead code in main. Consider driving it from a system property (e.g. Boolean.getBoolean("feldera.sql.roundTripCheck")) or wiring it to CompilerOptions so it can actually run in CI. As-is, it is a broken window — the next person reading this will assume it is being exercised, and the printer can silently regress without anyone noticing.
|
|
||
| /** Set the indent amount. If less or equal to 0, newline will have no effect. */ | ||
| @Override | ||
| public int getIndentAmount() { return this.amount; } |
There was a problem hiding this comment.
nit: the existing Javadoc /** Set the indent amount. ... */ on the line above is now attached to getIndentAmount() instead of setIndentAmount() (line 58). Either move that Javadoc down to where it belongs, or add a short /** @return spaces added per indent level. */ doc above this getter.
| case "sql_tsi_month": | ||
| case "sql_tsi_week": | ||
| case "sql_tsi_day": | ||
| case "sql_tsi_hour": |
There was a problem hiding this comment.
The new SQL_TSI_* keywords (plus the DATEDIFF/TIMESTAMPDIFF operator-table swap and ensureKeyword in ExpressionCompiler) are a non-trivial behavior change but I don't see a test that exercises DATEDIFF(SQL_TSI_DAY, ...) end-to-end. Worth a small case in PostgresTimestampTests or similar to lock the new path in, especially since the old improveErrorMessage rewrite is being removed at the same time.
| --png, -png | ||
| Emit a png image of the circuit instead of Rust | ||
| Default: false | ||
| --reformat |
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
APPROVE. Nice progress since my COMMENT review:
- Two of my three nits addressed: the
IIndentStream.getIndentAmount()doc /IndentStreamdoc positioning is fixed, and there's now a small compilation-failure test locking in the "Use TIMESTAMPDIFF instead of DATEDIFF for MINUTE" path. SqlComment+SqlCommentParserare new: clean, well-documented, correctly skip string literals and quoted identifiers, and consciously exclude Calcite hints (/*+ ... */) since those are already in the tree. TheMetadataTests.cleanupUdf()helper de-duplicates the truncate-file dance that used to be repeated inline — welcome tidy-up.
One remaining ask from my last review still applies: the final boolean ROUND_TRIP_CHECK = false; in SqlToRelCompiler.parseStatements() is still dead code. If it's a debug flag, driving it from a system property (Boolean.getBoolean("feldera.sql.roundTripCheck")) or a CompilerOptions field would let CI actually run it periodically. Otherwise it will silently rot and the next reader will assume the round-trip check is being exercised. Not a blocker.
Fixes #5214
Checklist