feat: Add a configuration property for Spark ANSI mode#14375
feat: Add a configuration property for Spark ANSI mode#14375philo-he wants to merge 3 commits intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| /// If true, Velox will use an ANSI-compliant dialect. For example, Velox will | ||
| /// throw a runtime exception instead of returning null results when the inputs | ||
| /// to a SQL operator or function are invalid. | ||
| static constexpr const char* kAnsiEnabled = "ansi_enabled"; |
There was a problem hiding this comment.
I assume this is specific to Spark functions. Presto functions are ANSI compliant already.
If so, let's add spark. prefix like other Spark-specific configs.
There is also a question about how to implement this without causing confusion or wrong results. It seems this change would allow user to request ANSI mode, but the results will not be compiant.
There was a problem hiding this comment.
@mbasmanova, thanks for your review. I will update the new config to indicate that it is intended for Spark use only.
There will be several upcoming PRs that rely on this configuration to adapt function implementations. The legacy mode and ANSI mode will coexist in Spark for a long period. We plan to introduce branching logic in relevant Velox functions, respecting this configuration to ensure ANSI compliance while minimizing code duplication. It's also possible to add separate ANSI-compliant functions if there isn't much shared code.
There was a problem hiding this comment.
This makes sense, but my question is about something else. How do we ensure that when ANSI is on the query returns correct results. If we land this change, then turning ANSI on will produce wrong results (same as with ANSI off).
One option is to create an allow list of functions and special forms that support ANSI mode and fail the query if ANSI mode is ON, but query contains a function that's not in allow list. We would need to think about where to place such a check.
CC: @rui-mo
There was a problem hiding this comment.
@mbasmanova, currently there is a rule in Gluten to fall back to vanilla Spark for the entire query when ANSI is on. Our plan is to firstly support a few common ANSI-compliant functions in Velox before removing this rule to allow the offloading. After this rule is removed, it's quite possible that incorrect results may be produced for functions that either lack ANSI support or have existing bugs, as you pointed out. To address this, maybe we can maintain a blacklist in Gluten to prevent the offloading for these functions, similar to our current approach when ANSI is off. This approach appears more user-friendly, as it allows the query to be executed in fallback mode rather than failing on Velox side.
There was a problem hiding this comment.
The ANSI mode-controlled scenarios are described in Spark's documentation at https://github.com/apache/spark/blob/v4.0.0/docs/sql-ref-ansi-compliance.md, and they have a variety of effects on the detailed implementation. Given this, it might be difficult to cover every aspect by adding a check in Velox.
Since Gluten currently fallback for ANSI on, it would more of an experimental feature. We can also add a document in Velox to list the ANSI-compatible functions when supporting them, which should be rather limited for now.
| - bool | ||
| - false | ||
| - If true, Velox will use an ANSI-compliant dialect. For example, Velox will throw a runtime exception instead of | ||
| returning null results when the inputs to a SQL operator or function are invalid. |
There was a problem hiding this comment.
Would you please mention current status of this config? I suppose it won't take effect for now.
There was a problem hiding this comment.
Do we have a sense of a timeline for updating ALL Spark functions to support this property? How many functions need updating? How do we plan to test this? Can we setup Fuzzer that runs with ansi on and compares the results with Spark?
I'm concerned about users starting to use this property and run into data correctness issues.
There was a problem hiding this comment.
@mbasmanova, my current estimate is at least one quarter to support all related Spark functions, depending on the number of contributors involved. I am still listing the functions that require ANSI support. Like non-ANSI functions, we will port Spark test cases into Velox and enable fuzzer tests to ensure consistency with Spark.
That would be helpful. Let's come up with a convention for how to indicate in the docs whether a function supports ANSI mode or not. Can we add an API that takes an Expr tree and returns true if all expressions are ANSI-compliant or not? This would allow non-Gluten users to check compliance easily. |
|
Does ANSI mode affect the behavior of any operators? Or is it only for functions? |
@mbasmanova, sounds great. I will evaluate the feasibility. Thanks for the suggestion.
To the best of my knowledge, ANSI mode primarily affects expressions. Maybe, one exception is implicit type coercion during write operations, which it can also influence. However, this effect is indirect—ANSI actually controls the underlying expressions that the write operation relies on. |
philo-he
left a comment
There was a problem hiding this comment.
@mbasmanova, I've updated this PR. Could you take a look again?
| static constexpr const char* kPrestoArrayAggIgnoreNulls = | ||
| "presto.array_agg.ignore_nulls"; | ||
|
|
||
| /// If true, Velox will use an ANSI-compliant dialect. For example, Velox will |
There was a problem hiding this comment.
It is not quite accurate to say "Velox will use" here because the setting applies only to Spark functions package, not to core engine. Let's change to something lilke
/// If true, Spark function's behavior is ANSI-compliant, e.g. throws runtime exception instead of returning null on invalid inputs.
Also, let's clarify that this setting is not implemented yet and provide guidance on how to tell which functions support this. Are we going to come up with some way to mark a function as ANSI-compliant in the docs?
There was a problem hiding this comment.
@mbasmanova, The suggestion makes sense. I've updated the PR accordingly. We will document the ANSI behavior in the existing documentation for the specific function as part of the ANSI-support PR. Thanks.
mbasmanova
left a comment
There was a problem hiding this comment.
Thanks.
We will document the ANSI behavior in the existing documentation for the specific function as part of the ANSI-support PR.
Let's come up with a standard way of doing that before PRs start showing up to ensure all PRs do that in a consistent easy to understand way.
CC: @rui-mo
@mbasmanova @philo-he I assume we could document the "standard way" at below with another PR, and follow this way in subsequent PRs. velox/velox/docs/spark_functions.rst Line 5 in 6bcbfe8 The standard way could be to add a label 
How do you think? |
|
@rui-mo I like this proposal. |
|
@peterenescu has imported this pull request. If you are a Meta employee, you can view this in D81253191. |
|
@peterenescu merged this pull request in 0570617. |
Summary: This is follow-up work for #14375 (comment). Pull Request resolved: #14698 Reviewed By: mbasmanova Differential Revision: D81962221 Pulled By: Yuhta fbshipit-source-id: 572ba75309b75a101410b569528037245f0d9178
Summary: This change prepares for supporting an ANSI-compliant dialect in Velox. The added configuration property allows function implementations to adapt based on the user's ANSI mode setting. See ANSI support discussion: facebookincubator/velox#3869. Pull Request resolved: facebookincubator/velox#14375 Reviewed By: Yuhta Differential Revision: D81253191 Pulled By: peterenescu fbshipit-source-id: a5f0212c95302721adaffdce7de512b160631646
This change prepares for supporting an ANSI-compliant dialect in Velox. The added
configuration property allows function implementations to adapt based on the user's
ANSI mode setting.
See ANSI support discussion: #3869.