Skip to content
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

[SPARK-50964][SQL] Use StaticInvoke to implement VariantGet codegen #49627

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Jan 23, 2025

What changes were proposed in this pull request?

The pr aims to

  • use StaticInvoke to implement VariantGet codegen.
  • support for non-literal paths in VariantGet.

Why are the changes needed?

Based on @cloud-fan's suggestion, using StaticInvoke to implement VariantGet codegen, it can simplify the code.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Pass GA.
  • Manually check, eg: VariantSuite, VariantExpressionSuite

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Jan 23, 2025
@panbingkun
Copy link
Contributor Author

panbingkun commented Jan 23, 2025

@chenhao-db
Copy link
Contributor

PushVariantIntoScanSuite is failing due to the change - it only recognizes VariantGet, but not the replaced expression (replacing runtime-replaceable expression happens before the rule). Essentially, I don't really think this change is a good idea. The implementation of VariantGet is simplified only by a bit, which is fixed and not likely to change, but the optimizer rules will get more complex (and there may be more rules targeting VariantGet in the future).

@panbingkun
Copy link
Contributor Author

PushVariantIntoScanSuite is failing due to the change - it only recognizes VariantGet, but not the replaced expression (replacing runtime-replaceable expression happens before the rule). Essentially, I don't really think this change is a good idea. The implementation of VariantGet is simplified only by a bit, which is fixed and not likely to change, but the optimizer rules will get more complex (and there may be more rules targeting VariantGet in the future).

I have fixed it, and another benefit of it is that when the path is foldable, it will run in ConstantFolding, thereby avoiding eval it every time and improving performance.

@cloud-fan

@panbingkun panbingkun requested a review from cloud-fan January 24, 2025 03:01
@panbingkun panbingkun marked this pull request as ready for review January 24, 2025 03:01
@@ -244,6 +272,24 @@ class VariantInRelation {
// Rewrite the attribute in advance, rather than depending on the last branch to rewrite it.
// Ww need to avoid the `v@StructPathToVariant(fields)` branch to rewrite the child again.
GetStructField(rewriteAttribute(v), fields(RequestedVariantField(g)))
case s @ StaticInvoke(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely the most annoying part of RuntimeReplaceable: pattern match becomes less intuitive.

Now I think it's probably better not to use RuntimeReplaceable for expressions that need be matched post analysis, until we improve RuntimeReplaceable to be replaced truly at runtime.

Copy link
Contributor Author

@panbingkun panbingkun Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hold it first, and I'll try to investigate how to make a truly RuntimeReplaceable (as we discussed privately, I think its idea is to only execute the replaced expression at runtime, while still maintaining it during the analysis and optimization stages, right?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that will be the best.

@panbingkun panbingkun marked this pull request as draft January 24, 2025 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants