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

[CALCITE-4025] Fix early exit in DelegatingScope#fullyQualify #2112

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

laurentgo
Copy link
Contributor

Address an issue in fullyQualify method when fully qualifying an
identifier and the prefix cannot be resolved but could if name matching
was not case sensitive, the method would throw an exception instead of
trying to continue matching smaller prefixes (in case of compound
columns keys).

Address an issue in fullyQualify method when fully qualifying an
identifier and the prefix cannot be resolved but could if name matching
was not case sensitive, the method would throw an exception instead of
trying to continue matching smaller prefixes (in case of compound
columns keys).
@zinking
Copy link
Contributor

zinking commented Aug 20, 2020

LGTM, except I would "" empty string instead of null

@laurentgo
Copy link
Contributor Author

I'm assuming you meant for this piece of code?

      // to capture possible suggestion if no table alias found
      SqlIdentifier suggestionPrefix = null;
      String suggestionName = null;

I'm not sure why an empty string would be a better choice here. There's no safety issue, and null captures the absence of a match clearly (vs a suggestion which would be an empty table name).

Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants