[WIP] Preparser #52085
Open
[WIP] Preparser #52085
+2,897
−251
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
This commit moves the core parameter substitution parser implementation from the parmsubstitution branch to the preparser branch: - SubstituteParamsParser: Main parser for substituting parameter markers in SQL - SubstituteParmsAstBuilder: AST builder for extracting parameter locations The usage code (BindParameters rule) remains in the parmsubstitution branch as requested, only the core parser implementation is moved.
This commit implements parameter substitution as a pre-processing step in the SparkSqlParser, allowing parameter markers to be substituted before the main SQL parser executes. Key changes: 1. Added ParameterContext classes and ThreadLocal management for passing parameter values to the parser 2. Modified SparkSqlParser to perform parameter substitution before variable substitution and main parsing 3. Updated SparkSession sql methods to use new parameter substitution approach via ThreadLocal context The flow is now: 1. SparkSession sets parameter context in ThreadLocal 2. SparkSqlParser detects and substitutes parameter markers 3. Variable substitution happens on the result 4. Main ANTLR parsing proceeds with fully substituted SQL This eliminates the need for ParameterizedQuery LogicalPlan nodes and moves parameter handling to the parsing phase where it belongs.
- Remove unused Literal import - Fix trailing whitespace issues
Added fully qualified class names for: - SubstituteParamsParser - ThreadLocalParameterContext - ParameterContext - SubstitutionRule - NamedParameterContext - PositionalParameterContext This should resolve the compilation issues preventing parameter substitution from working.
This will help us understand why parameter substitution is not working: - Added debug logging in parse() method to show parameter context - Added debug logging in substituteParametersIfNeeded() to trace execution - Will help identify if context is being set or if substitution is failing
Removed trailing whitespace from lines in the parameter substitution integration code. Future commits will ensure all files are free of trailing whitespace.
- Broke long import statement across multiple lines (imports should be < 100 chars) - Split long ThreadLocalParameterContext.get() call across lines - All lines now comply with 100-character limit This establishes the coding style guideline: - Maximum line length: 100 characters - Break long imports and method calls appropriately - Use proper indentation for continuation lines
Created CODING_STYLE_GUIDELINES.md with rules for: - Maximum line length: 100 characters - No trailing whitespace - Proper import statement formatting - Method definition formatting - Pattern matching formatting - Pre-commit checks and tools - Examples of common fixes This establishes consistent standards for all future code changes and provides clear guidelines for maintaining code quality.
The parameter substitution integration has been verified to work correctly:
- Named parameters are properly detected and substituted
- Parameter values are correctly converted to SQL literals
- Integration flow: SparkSession -> ThreadLocal -> SparkSqlParser -> SubstituteParamsParser
Example working usage:
spark.sql("create or replace view v(c1) AS VALUES (:parm)", Map("parm" -> "hello"))
Successfully substitutes :parm with 'hello' before main parsing.
srielau
force-pushed
the
preparser
branch
2 times, most recently
from
816a26f to
e415f4f
Compare
cloud-fan
reviewed
Aug 21, 2025
CODING_STYLE_GUIDELINES.md
Outdated
| @@ -0,0 +1,127 @@ | |||
| # Coding Style Guidelines | |||
There was a problem hiding this comment.
lol, yes I remove it
cloud-fan
reviewed
Aug 21, 2025
| | namedParameterMarkerVal | ||
| ; | ||
|
|
||
| namedParameterMarkerVal |
There was a problem hiding this comment.
Actually it is not. Because without pushing it down we need visits for stringLitWithoutMarker (either all or none)
cloud-fan
reviewed
Aug 21, 2025
| if (ctx.RECURSIVE() == null) { | ||
| operationNotAllowed("Cannot specify MAX RECURSION LEVEL when the CTE is not marked as " + | ||
| "RECURSIVE", ctx) | ||
| } | ||
| Some(nCtx.INTEGER_VALUE().getText().toInt) | ||
| Some(nCtx.integerValue().getText().toInt) |
There was a problem hiding this comment.
in the normal parser, shall we fail in integerValue if parameter marker is found? it should be replaced by the pre-parser already.
…d named, unnamed errors.
This file was not related to the parameter substitution feature and should not be part of this branch.
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.
What changes were proposed in this pull request?
We are proposing to greatly expand the availability of named parameter markers to virtually all places literals are allowed.
Th eway to do this is by adding a pre-processing parser which's sole purpose is to find named parameter markers and replace them with literals without having to impact the grammar or analyzer profoundly.
Why are the changes needed?
Users have shown a desire to parameterize not only ezpressions in queries, but also DDL scripts and utility commands.
Many such uses, such as COMMENT and TBLPROPERTIES do not get processed by Analyzer rules.
Does this PR introduce any user-facing change?
Yes, it expands teh usage of named parameter markers.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
Yes, it was co-authored by cursor.