-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-53573][SQL] Use Pre-processor for generalized parameter marker handling #52334
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
Closed
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
Closed
cloud-fan
reviewed
Sep 24, 2025
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/SubstituteParmsAstBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/SubstituteParmsAstBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/SubstituteParmsAstBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParameterContext.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParameterHandler.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParameterHandler.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParameterHandler.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/LiteralToSqlConverter.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/LiteralToSqlConverter.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 24, 2025
sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala
Outdated
Show resolved
Hide resolved
This commit implements a comprehensive parameter substitution system for Spark SQL that provides detailed error context for EXECUTE IMMEDIATE statements, similar to how views handle errors. Key features: - Pre-parser approach with position mapping for accurate error reporting - Thread-local parameter context for parser-aware error handling - Support for both named and positional parameters across all SQL APIs - Optimized position mapping algorithm (O(n²) → O(k) where k = substitutions) - Comprehensive test coverage including edge cases and error scenarios - Backward compatibility with legacy parameter substitution mode The implementation includes: - ParameterHandler for unified parameter processing - PositionMapper for efficient error position translation - LiteralToSqlConverter for type-safe SQL generation - Integration with SparkSqlParser, SparkSession, and EXECUTE IMMEDIATE - Enhanced error messages showing both outer and inner query context This addresses the user request for detailed error context in EXECUTE IMMEDIATE statements while maintaining performance and compatibility.
efbd9cf
to
4fe9577
Compare
a238338
to
20e66b0
Compare
cloud-fan
reviewed
Oct 15, 2025
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 15, 2025
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkParserUtils.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 15, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 15, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParameterHandler.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParameterHandler.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParameterContext.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/SubstituteParamsParser.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/QueryPlanSuite.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Oct 16, 2025
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
Outdated
Show resolved
Hide resolved
We reviewed this extensively, the architecture is sound and the test coverage is thorough. The one remaining CI failure is |
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?
This PR introduces a comprehensive parameter substitution system that significantly expands parameter marker support and implements major performance optimizations. The current implementation of parameter markers (
:param
and?
) is restricted to expressions and queries. This PR enables parameter markers to work universally across all SQL constructs.Key Changes
Enhanced Parameter Substitution Architecture:
ParameterHandler
class with centralized, optimized parameter substitution logicSparkSqlParser
with parser-aware error contextParameterSubstitutionContext
with thread-local substitution informationEXECUTE IMMEDIATE Integration & Error Context:
ResolveExecuteImmediate
with comprehensive parameter validation and local variable isolationHybridParameterContext
supporting both named and positional parameters with strict validationEXECUTE IMMEDIATE
from accessing outer SQL scripting variablesLegacy Mode Support:
spark.sql.legacy.parameterSubstitution.constantsOnly
Why are the changes needed?
Functional Limitations:
The enhanced architecture solves these by:
Does this PR introduce any user-facing changes?
Yes - Significantly Expanded Functionality:
New Parameter Support:
CREATE VIEW v(c1) AS (SELECT :p)
SHOW TABLES FROM schema LIKE :pattern
DECIMAL(?, ?)
TBLPROPERTIES (':key' = ':value')
Full Backward Compatibility:
How was this patch tested?
Comprehensive Unit Testing:
ParametersSuite
with 50+ test cases covering all parameter scenariosParameterSubstitutionSuite
for algorithm-specific testingSqlScriptingExecutionSuite
tests for EXECUTE IMMEDIATE isolationIntegration Testing:
Manual Verification:
CREATE TABLE :name (id INT)
EXECUTE IMMEDIATE 'SELECT typeof(:p), :p' USING 5::INT AS p
Example Enhanced Capabilities:
Quality Assurance: