Skip to content

Fix assertion error in sql planning for latest aggregators#13151

Merged
abhishekagarwal87 merged 4 commits intoapache:masterfrom
abhishekagarwal87:sql_planning_latest_bug
Sep 28, 2022
Merged

Fix assertion error in sql planning for latest aggregators#13151
abhishekagarwal87 merged 4 commits intoapache:masterfrom
abhishekagarwal87:sql_planning_latest_bug

Conversation

@abhishekagarwal87
Copy link
Copy Markdown
Contributor

@abhishekagarwal87 abhishekagarwal87 commented Sep 28, 2022 •
edited
Loading

Fixes #13091.

This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.1 milestone Sep 28, 2022
cryptoe
cryptoe approved these changes Sep 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@cryptoe cryptoe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.
+1

kfaraz
kfaraz reviewed Sep 28, 2022
View reviewed changes
Comment thread ...in/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java Outdated
maxStringBytes = RexLiteral.intValue(rexNodes.get(1));
}
catch (AssertionError ae) {
plannerContext.setPlanningError("The second argument '%s' to '%s' function is not a literal", aggregateCall.getName(), rexNodes.get(1));
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

The arguments to this formatted error message seem reversed.

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

We can also safely say that the argument should be an integer (maybe also mention the name of the argument) rather than saying that it should be a "literal".

kfaraz
kfaraz requested changes Sep 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left some comments.

kfaraz
kfaraz reviewed Sep 28, 2022
View reviewed changes
Comment thread sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
}

@Test
public void testStringLatestByGroupByWithAlwaysFalseCondition()
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

Nit: Maybe also add a test case to verify error message?

Copy link
Copy Markdown
Contributor Author

@abhishekagarwal87 abhishekagarwal87 Sep 28, 2022

Choose a reason for hiding this comment

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

I was not able to figure out a query that gets me that error message. This query is not an invalid query. It just goes through a bad stage and doesn't recover before this fix. Now it can.

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

Perhaps a LATEST(dim4, "100") but it's not essential, we can do it later.

Copy link
Copy Markdown
Contributor Author

@abhishekagarwal87 abhishekagarwal87 Sep 28, 2022

Choose a reason for hiding this comment

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

that SQL will generate a different error and fails even before reaching the SqlAggregagtor.

kfaraz
kfaraz reviewed Sep 28, 2022
View reviewed changes
Comment thread ...ain/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java
case 3:
int maxStringBytes;
try {
maxStringBytes = RexLiteral.intValue(rexNodes.get(2));
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

From the docs, it seems that maxBytesPerString is always the second argument, i.e. rexNodes.get(1)

Copy link
Copy Markdown
Contributor Author

@abhishekagarwal87 abhishekagarwal87 Sep 28, 2022

Choose a reason for hiding this comment

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

For EARLIEST_BY/LATEST_BY, it's the third argument. For non _BY aggregators, it's second argument.

Copy link
Copy Markdown
Contributor Author

@abhishekagarwal87 abhishekagarwal87 Sep 28, 2022

Choose a reason for hiding this comment

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

I fixed the error message though.

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

Thanks for the clarification, I was looking at an older doc in the original PR.

@abhishekagarwal87
Copy link
Copy Markdown
Contributor Author

abhishekagarwal87 commented Sep 28, 2022

Thank you @kfaraz and @cryptoe for your review. I have made the changes.

kfaraz
kfaraz approved these changes Sep 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, @abhishekagarwal87 !

Comment thread ...ain/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java
case 3:
int maxStringBytes;
try {
maxStringBytes = RexLiteral.intValue(rexNodes.get(2));
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

Thanks for the clarification, I was looking at an older doc in the original PR.

Comment thread sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
}

@Test
public void testStringLatestByGroupByWithAlwaysFalseCondition()
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Sep 28, 2022

Choose a reason for hiding this comment

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

Perhaps a LATEST(dim4, "100") but it's not essential, we can do it later.

@abhishekagarwal87 abhishekagarwal87 merged commit 61b3495 into apache:master Sep 28, 2022
@abhishekagarwal87
Copy link
Copy Markdown
Contributor Author

abhishekagarwal87 commented Sep 28, 2022

Merged since build failure is unrelated.

@abhishekagarwal87 abhishekagarwal87 deleted the sql_planning_latest_bug branch September 28, 2022 15:31
abhishekagarwal87 added a commit that referenced this pull request Sep 28, 2022
@abhishekagarwal87
Fix assertion error in sql planning for latest aggregators (#13151)
94e3673 
* Fix sql planning bug for latest aggregators

* change test name

* Fix error messages

* fix error message again
@kfaraz kfaraz mentioned this pull request Nov 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cryptoe cryptoe cryptoe approved these changes

@kfaraz kfaraz kfaraz approved these changes

Assignees

No one assigned

Labels

Area - SQL

Projects

None yet

Milestone

24.0.1

Development

Successfully merging this pull request may close these issues.

Druid query returns 500 error with javax.servlet.ServletException: java.lang.AssertionError: not a literal: $2

3 participants

@abhishekagarwal87 @cryptoe @kfaraz