You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Steve Carlin (Code Review)" <ge...@cloudera.org> on 2024/07/01 20:22:37 UTC

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Steve Carlin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21237


Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................

IMPALA-12954: Implement Sorting capability for Calcite planner

The Sort RelNode is now supported. This includes limit an offset features
as well.

A minor bit of code was copied from the original planner, but it just
involved decision making on which Sort PlanNode to call, so this probably
doesn't need to be refactored.

Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
---
M java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
M testdata/workloads/functional-query/queries/QueryTest/calcite.test
6 files changed, 288 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/37/21237/9
-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 9
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 11: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test@198
PS10, Line 198: 4,0
> Not sure I understand this. There is an order by clause (order by abs(bigin
Oh oops, I missed it was doing order by on both.



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jul 2024 23:05:47 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 13:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10872/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Aug 2024 20:14:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16454/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 9
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jul 2024 20:47:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21237

to look at the new patch set (#12).

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................

IMPALA-12954: Implement Sorting capability for Calcite planner

The Sort RelNode is now supported. This includes limit an offset features
as well.

A minor bit of code was copied from the original planner, but it just
involved decision making on which Sort PlanNode to call, so this probably
doesn't need to be refactored.

Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
---
M java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
M testdata/workloads/functional-query/queries/QueryTest/calcite.test
6 files changed, 371 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/37/21237/12
-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 12
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 9:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java:

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@51
PS9, Line 51:       .put(SqlKind.GREATER_THAN, "gt")
Could we add a full set of binary comparison functions here and test them with this patch?


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java:

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@103
PS9, Line 103:     // If there's limit without order-by, we don't need to generate
So Calcite's Sort node covers both limit and order-by?


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@146
PS9, Line 146:     return context.filterCondition_ != null
Another use for wrapInSelectNodeIfNeeded when you rebase.


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@177
PS9, Line 177:    * do not necessarily line up with the indexes.   So we need to walk through the
nit: extra spaces


http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test@195
PS9, Line 195: # sort test
Can we test all the variations in https://impala.apache.org/docs/build/asf-site-html/topics/impala_order_by.html? Ascend/descend, nulls, order by column ordinal:

  ORDER BY col_ref [, col_ref ...] [ASC | DESC] [NULLS FIRST | NULLS LAST]



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 9
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Jul 2024 18:27:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 12: Code-Review+2

Thanks, this looks good


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 12
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Aug 2024 19:35:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 13: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Aug 2024 21:36:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 13: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Aug 2024 01:14:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test@198
PS10, Line 198: 4,0
> Is the sort order if id deterministic? It's not specified in the query. Not
Did you get a chance to look at this question?



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jul 2024 21:42:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................

IMPALA-12954: Implement Sorting capability for Calcite planner

The Sort RelNode is now supported. This includes limit an offset features
as well.

A minor bit of code was copied from the original planner, but it just
involved decision making on which Sort PlanNode to call, so this probably
doesn't need to be refactored.

Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Reviewed-on: http://gerrit.cloudera.org:8080/21237
Reviewed-by: Michael Smith <mi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
M testdata/workloads/functional-query/queries/QueryTest/calcite.test
6 files changed, 371 insertions(+), 0 deletions(-)

Approvals:
  Michael Smith: Looks good to me, approved
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 14
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21237

to look at the new patch set (#11).

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................

IMPALA-12954: Implement Sorting capability for Calcite planner

The Sort RelNode is now supported. This includes limit an offset features
as well.

A minor bit of code was copied from the original planner, but it just
involved decision making on which Sort PlanNode to call, so this probably
doesn't need to be refactored.

Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
---
M java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
M testdata/workloads/functional-query/queries/QueryTest/calcite.test
6 files changed, 377 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/37/21237/11
-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test@198
PS10, Line 198: 4,0
> Is the sort order if id deterministic? It's not specified in the query. Not
Not sure I understand this. There is an order by clause (order by abs(bigint_col), id) for this query which makes it deterministic, no?  Am I missing something?



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 10
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Jul 2024 23:04:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java:

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@153
PS9, Line 153:   private NodeWithExprs getChildPlanNode(ParentPlanRelContext context) throws ImpalaException {
line too long (95 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 9
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jul 2024 20:23:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 11:

(4 comments)

This looks good, just a couple small nits

http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java:

http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@166
PS11, Line 166: validateNoOrderBySort
Nit: I think something like validateUnorderedLimit() or validateUnsortedLimit() would be more descriptive.


http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@222
PS11, Line 222: boolean disableTopN
Nit: Is there a place where we set this to true?


http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@229
PS11, Line 229:     long topNBytesLimit = planCtx.getQueryOptions().topn_bytes_limit;
              :     if (topNBytesLimit <= 0) {
              :       return SortNode.createTopNSortNode(planCtx.getQueryOptions(),
              :           planCtx.getNextNodeId(), root, sortInfo, offset, limit, false);
              :     }
              : 
              :     long topNCardinality =
              :         PlanNode.capCardinalityAtLimit(root.getCardinality(), limit);
              :     long estimatedTopNMaterializedSize =
              :         sortInfo.estimateTopNMaterializedSize(topNCardinality, offset);
              : 
              :     return estimatedTopNMaterializedSize < topNBytesLimit
              :         ? SortNode.createTopNSortNode(planCtx.getQueryOptions(),
              :           planCtx.getNextNodeId(), root, sortInfo, offset, limit, false)
              :         : SortNode.createTotalSortNode(planCtx.getNextNodeId(), root,
              :               sortInfo, offset);
Nit: This logic looks very similar to the logic inside SortNode.createTopNSortNode(), which also does checks to see if it should use a total sort. Is it possible for us to just call SortNode.createTopNSortNode() and rely on the logic inside there?


http://gerrit.cloudera.org:8080/#/c/21237/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test@291
PS11, Line 291: # limit test
              : select id, abs(bigint_col) from functional.alltypestiny where id > 2 order by abs(bigint_col), id limit 3;
Could we also add a test case for a simple limit without an order by?



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Aug 2024 00:13:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 12:

Looks like this needs a manual rebase after the parent was merged.


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 12
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Aug 2024 21:24:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java:

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@51
PS9, Line 51:       .put(SqlKind.GREATER_THAN, "gt")
> I added the basic ones, but not all.  I did not include the distinct ones n
That seems fine. I realize this was primarily only needed for sorting, and wasn't sure whether we'd also need "lt" to implement ASC/DESC.


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java:

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@146
PS9, Line 146:     // If there is a filter condition, a SelectNode will get added on top
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@177
PS9, Line 177:    * do not necessarily line up with the indexes. So we need to walk through the
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test@195
PS9, Line 195: # sort test
> I mentioned this for an above comment...there were so few of these that I w
Thanks. I agree there's a balance to strike between testing for Calcite specifically, and working towards getting the full test suite working. Easy coverage is good though, and I'm not sure how thorough some of our existing testing is.


http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test@198
PS10, Line 198: 4,0
Is the sort order if id deterministic? It's not specified in the query. Not sure whether the test can assume that or needs to be explicit. I think this may be ok to assume for NUM_NODES=1, but not in a distributed plan.



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 10
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2024 18:14:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 10:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/16480/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 10
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2024 17:01:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21237

to look at the new patch set (#10).

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................

IMPALA-12954: Implement Sorting capability for Calcite planner

The Sort RelNode is now supported. This includes limit an offset features
as well.

A minor bit of code was copied from the original planner, but it just
involved decision making on which Sort PlanNode to call, so this probably
doesn't need to be refactored.

Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
---
M java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
M testdata/workloads/functional-query/queries/QueryTest/calcite.test
6 files changed, 376 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/37/21237/10
-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 10
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 12:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16686/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 12
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Aug 2024 18:02:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Joe McDonnell, Csaba Ringhofer, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21237

to look at the new patch set (#13).

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................

IMPALA-12954: Implement Sorting capability for Calcite planner

The Sort RelNode is now supported. This includes limit an offset features
as well.

A minor bit of code was copied from the original planner, but it just
involved decision making on which Sort PlanNode to call, so this probably
doesn't need to be refactored.

Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
---
M java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
M java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
M testdata/workloads/functional-query/queries/QueryTest/calcite.test
6 files changed, 371 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/37/21237/13
-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 13:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16711/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Aug 2024 20:04:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 9:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java:

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@51
PS9, Line 51:       .put(SqlKind.GREATER_THAN, "gt")
> Could we add a full set of binary comparison functions here and test them w
I added the basic ones, but not all.  I did not include the distinct ones nor the "nor equals"

Eventually everything should be tested once we have all the Impala tests run through the Calcite planner so I think this is ok for now?  Thoughts?


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java:

http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@103
PS9, Line 103:     // If there's limit without order-by, we don't need to generate
> So Calcite's Sort node covers both limit and order-by?
Correct.  Do you think I should make more general changes namewise to express that?


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@146
PS9, Line 146:     return context.filterCondition_ != null
> Another use for wrapInSelectNodeIfNeeded when you rebase.
Done


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@153
PS9, Line 153:   private NodeWithExprs getChildPlanNode(ParentPlanRelContext context) throws ImpalaException {
> line too long (95 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@177
PS9, Line 177:    * do not necessarily line up with the indexes.   So we need to walk through the
> nit: extra spaces
Done


http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test@195
PS9, Line 195: # sort test
> Can we test all the variations in https://impala.apache.org/docs/build/asf-
I mentioned this for an above comment...there were so few of these that I went ahead and did them.

In general, I figure that once all the Impala tests are enabled, it will cover all variations so I'm not too worried about being overly comprehensive.



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 9
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2024 16:45:54 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 11:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16485/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2024 22:36:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12954: Implement Sorting capability for Calcite planner

Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )

Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner
......................................................................


Patch Set 11:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java
File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java:

http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@166
PS11, Line 166: validateNoOrderBySort
> Nit: I think something like validateUnorderedLimit() or validateUnsortedLim
Done


http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@222
PS11, Line 222: boolean disableTopN
> Nit: Is there a place where we set this to true?
Remnants of some deprecated code, deleted.


http://gerrit.cloudera.org:8080/#/c/21237/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@229
PS11, Line 229:     long topNBytesLimit = planCtx.getQueryOptions().topn_bytes_limit;
              :     if (topNBytesLimit <= 0) {
              :       return SortNode.createTopNSortNode(planCtx.getQueryOptions(),
              :           planCtx.getNextNodeId(), root, sortInfo, offset, limit, false);
              :     }
              : 
              :     long topNCardinality =
              :         PlanNode.capCardinalityAtLimit(root.getCardinality(), limit);
              :     long estimatedTopNMaterializedSize =
              :         sortInfo.estimateTopNMaterializedSize(topNCardinality, offset);
              : 
              :     return estimatedTopNMaterializedSize < topNBytesLimit
              :         ? SortNode.createTopNSortNode(planCtx.getQueryOptions(),
              :           planCtx.getNextNodeId(), root, sortInfo, offset, limit, false)
              :         : SortNode.createTotalSortNode(planCtx.getNextNodeId(), root,
              :               sortInfo, offset);
> Nit: This logic looks very similar to the logic inside SortNode.createTopNS
Done


http://gerrit.cloudera.org:8080/#/c/21237/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test
File testdata/workloads/functional-query/queries/QueryTest/calcite.test:

http://gerrit.cloudera.org:8080/#/c/21237/11/testdata/workloads/functional-query/queries/QueryTest/calcite.test@291
PS11, Line 291: # limit test
              : select id, abs(bigint_col) from functional.alltypestiny where id > 2 order by abs(bigint_col), id limit 3;
> Could we also add a test case for a simple limit without an order by?
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/21237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5
Gerrit-Change-Number: 21237
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Aug 2024 17:38:31 +0000
Gerrit-HasComments: Yes