Skip to content

[CALCITE-6641] Compiling programs with ASOF joins can report obscure errors#4015

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue6641
Oct 26, 2024
Merged

[CALCITE-6641] Compiling programs with ASOF joins can report obscure errors#4015
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue6641

Conversation

@mihaibudiu
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

@caicancai caicancai left a comment

Choose a reason for hiding this comment

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

I don't really understand this part, but the code looks good

@Override public @Nullable SqlNode visit(final SqlCall call) {
SqlKind kind = call.getKind();
if (kind != SqlKind.AND && kind != SqlKind.EQUALS) {
if (kind != SqlKind.AND && kind != SqlKind.EQUALS && kind != SqlKind.CAST) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice if you could explain why SqlKind cannot be equal to these types

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The JavaDoc says "shuttle which determines whether an expression is a simple conjunction of equalities". AND is conjunction, and EQUALS is equalities. CAST is only allowed when applied to field names.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks

SqlKind kind = call.getKind();
if (kind != SqlKind.AND && kind != SqlKind.EQUALS) {
if (kind != SqlKind.AND && kind != SqlKind.EQUALS && kind != SqlKind.CAST) {
illegal = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I check the checkAnd(call) and this method, Maybe we can exit directly when we find the illegal value is true.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor advice:

  1. SqlValidatorImpl#validateJoin(Line#3784). There is an extra {} behind the ASOF.
  2. ConjunctionOfEqualities#checkAnd method's java comment. Before Children There is an extra space.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the extra {} are actually useful, they create a local scope, and the variables in the local scope do not interfere with variables from the rest of the function. It also prevents accidental updates to other variables that may be in scope. If also helps with the debugger too: such variables go out of scope when the inner scope {} terminates, so in large functions with lots of variables it makes the debugging experience better. I use this trick a lot when I have to write very large functions.

…errors

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud
Copy link
Copy Markdown

@mihaibudiu mihaibudiu merged commit 5b4f32a into apache:main Oct 26, 2024
@mihaibudiu mihaibudiu deleted the issue6641 branch October 26, 2024 17:11
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.

3 participants