[SQL] Do not preserve source positions for internally-generated SQL #5308
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the distinction between internal and external source positions in the SQL compiler to prevent internally-generated SQL from polluting profiler views with incorrect source code origins. The compiler now marks source positions as "internal" for SQL code it generates itself (such as error view definitions and UDF implementations), ensuring only user-written code positions are propagated externally.
Key Changes:
- Introduced
ExtendedSqlParserPosto track whether source positions are internal or external - Fixed multiple Calcite SQL node classes to properly implement visitor APIs by overriding
createCall()andgetOperandList()methods - Updated
SourcePositionRangeto filter out internal positions when requested
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| metadataTests-generateDFRecursive.json | Test output showing empty positions array for internal ERROR_VIEW |
| metadataTests-generateDF.json | Test output showing empty positions array for internal ERROR_VIEW |
| CreateTypeStatement.java | Import path change to use custom SqlCreateType |
| SqlViewColumnDeclaration.java | Fixed getOperandList() to include all operands and added createCall() override |
| SqlRemove.java | Fixed getOperandList() to include all three operands |
| SqlPrimaryKey.java | New class replacing Calcite's SqlKeyConstraint with proper visitor support |
| SqlLateness.java | Removed empty TODO validation method |
| SqlForeignKey.java | Added createCall() override for proper visitor support |
| SqlExtendedColumnDeclaration.java | Fixed getOperandList() to include all operands, added createCall() override and constructor |
| SqlDropView.java | New class with proper createCall() override |
| SqlDropTable.java | New class with proper createCall() override |
| SqlDropObject.java | New base class for drop operations |
| SqlDeclareView.java | Added createCall() override for proper visitor support |
| SqlCreateView.java | Added createCall() override, clone() method, and fixed getOperandList() |
| SqlCreateType.java | New class replacing Calcite's version with proper visitor support |
| SqlCreateTable.java | Added createCall() override, clone() method, and fixed getOperandList() |
| SqlCreateIndex.java | Added createCall() override for proper visitor support |
| SqlCreateFunctionDeclaration.java | Added createCall() override and fixed getOperandList() |
| SqlCreateAggregate.java | Added createCall() override and fixed getOperandList() |
| SqlAttributeDefinition.java | New class replacing Calcite's version with proper visitor support |
| SqlToRelCompiler.java | Added ReplacePositions visitor to mark internal positions and updated parsing flow |
| ExtendedSqlParserPos.java | New class extending SqlParserPos to track internal/external distinction |
| CalciteToDBSPCompiler.java | Import path changes to use custom SQL classes |
| SourcePositionRange.java | Added logic to skip internal positions when requested |
| ddl.ftl | Updated parser template to use custom classes instead of Calcite's SqlDdlNodes |
| config.fmpp | Updated imports to include custom SQL classes |
mihaibudiu
force-pushed
the
noposition
branch
from
78aa027 to
c919375
Compare
abhizer
approved these changes
Dec 20, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Contributor
Author
|
This needs apache/calcite#4696 to be merged first |
Signed-off-by: Mihai Budiu <[email protected]>
mihaibudiu
force-pushed
the
noposition
branch
from
c919375 to
94f42f9
Compare
Signed-off-by: Mihai Budiu <[email protected]>
mihaibudiu
force-pushed
the
noposition
branch
from
94f42f9 to
b4706df
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The compiler generates some dataflow graph fragments by generating SQL strings and recursively invoking itself on these SQL program. These dataflow graph fragments are then stitched into the final dataflow graph.
For example, the error view is obtained by the following declaration:
The
FELDERA_ERROR_TABLEis deleted later and the view is hooked to the rightful sources.SQL User-defined functions are implemented in a similar way.
An unpleasant side-effect of this process is that the source code propagation mechanism of the compiler records source positions for these fictitious programs, which then surface while browsing the profiles (where they point to incorrect source code origins, since these fictitious programs are not recorded in the actual program sources).
This PR distinguished two kinds of source positions: internal and external. Only the external ones are propagated to the outside world.
Unfortunately this PR is much larger than I expected, because I realized that many Calcite classes incorrectly implement the APIs used by the Calcite visitors, and I am using a visitor to rewrite the position information. As a consequence I had to clone and fix all the Calcite classes in question. I may file an issue for Calcite as well, but no one seems to have noticed this problem so far.
The final two resource files in this PR show indeed that the source positions are no longer present for the
ERROR_VIEW.