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

Replace FunctionFieldReferenceOperator with Calcite DOT operator #523

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

KevinGe00
Copy link
Contributor

@KevinGe00 KevinGe00 commented Aug 5, 2024

What changes are proposed in this pull request, and why are they necessary?

We can get rid of FunctionFieldReferenceOperator entirely and replace it with Calcite's default SqlDotOperator. Historically FunctionFieldReferenceOperator was introduced because "Calcite DOT operator by default does not work for referencing fields from struct return types of functions." However, that's no longer the case, thus rendering FunctionFieldReferenceOperator unneeded. If we can get rid of FunctionFieldReferenceOperator, we also no longer need to migrate it (as part of Coral IR upgrades).

How was this patch tested?

  • clean build
  • ran regression tests for avro, trino and spark
    Only regression I'm seeing is a cosmetic one that is actually desirable. Unnecessary parentheses introduced because FunctionFieldReferenceOperator is not a native Calcite operator go away:
old: (coalesce_struct(table.struct_col).tag_1).field
new: coalesce_struct(table.struct_col).tag_1.field

Copy link
Contributor

@aastha25 aastha25 left a comment

Choose a reason for hiding this comment

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

  1. Have we verified that the cosmetic changes have no impact when queried on Spark/Trino? This should be a simple experiment to execute the two variants on Spark/Trino for a sample table.

  2. Could you also confirm the the RelNode representations have no differences apart from the operator name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants