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 2025/01/06 19:30:28 UTC
[Impala-ASF-CR] WIP: do not review
Steve Carlin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22306
Change subject: WIP: do not review
......................................................................
WIP: do not review
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreator.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreatorImpl.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
21 files changed, 697 insertions(+), 218 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/1
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@54
PS11, Line 54: public SingleNodePlannerIntf createSingleNodePlanner(PlannerContext ctx)
: throws ImpalaException;
:
: /**
: * Return a fallback compiler factory given a planner context allowing the
: * implementation to choose if it should fallback based on a query option or
> I would do something like this (which might fit better in the Calcite add-o
Ok, did something close to this. Moved it into the other patch.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 18
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Mar 2025 01:58:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#21).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
17 files changed, 515 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/21
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 21
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 22:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18700/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 22
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 01:10:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 8:
(2 comments)
I wanted to see what was possible here, so I hacked on an alternate version of this. It reuses most of the concepts here, except that it subclasses AnalysisContext/AnalysisResult directly rather than having an AnalysisDriver. It also has a single factory rather than using chaining. I didn't spend time on the frontend tests except to get them to compile, but it shouldn't be hard to use the factory in the frontend tests. I moved the fallback to what I think is the right place (Frontend::createExecRequest()). Take a look here: http://gerrit.cloudera.org:8080/22475
Based on my exploration, I don't think the AnalysisDriver is needed. I'm a bit ambivalent about chaining versus factory, but I do think it is awkward to have a ParsedStatement know how to instantiate an AnalysisContext and an AnalysisResult know how to instantiate a SingleNodePlanner.
http://gerrit.cloudera.org:8080/#/c/22306/8/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/8/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@490
PS8, Line 490: // TODO: hack! Having the singleNodePlanner_ set within the analysisResult
: // is not a good thing to do. It really doesn't belong here.
: // However, the TResultSetMetadata for CalcitePlanner is only created after the
: // single node planner is run, but the regular Impala planner fetches the info
: // off of AnalysisResult in a place where the SingleNodePlanner is not available.
: // Some major refactoring should be done to make this cleaner in order for both
: // compilation paths to work, but in the meantime, we set the SingleNodePlanner
: // within the AnalysisResult so that the TResultSetMetadata can be fetched from
: // one place.
Here are two options:
1. getPlannedExecRequest()/createExecRequest() could return a structure that contains a TQueryExecRequest and other things like the TResultSetMetadata (only set for queries). My guess is that this could be a useful concept. There are other things that use Analyzer/AnalysisResult as plumbing out of Planner, and this could avoid that.
2. Frontend's PlanCtx (which probably should have a different name, given how close it is to PlannerContext) is being passed through these functions and can be used to pass information out. Basically, if we have a TResultSetMetadata field that is set in createExecRequest() only for queries, we can read it back in that location in doCreateExecRequest().
The second is hackier, but easier.
http://gerrit.cloudera.org:8080/#/c/22306/8/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@585
PS8, Line 585: public static class AnalysisDriverImpl implements AnalysisDriver {
I don't think the AnalysisDriver concept is necessary. It would be better to reuse the AnalysisContext / AnalysisResult concepts. I'm posting some code to show one way to do that. Basically, CalciteAnalysisContext subclasses AnalysisContext and CalciteAnalysisResult subclasses AnalysisResult.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 8
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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, 11 Feb 2025 23:54:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@54
PS11, Line 54: /**
: * Return a fallback compiler factory given a planner context allowing the
: * implementation to choose if it should fallback based on a query option or
: * the type of exception thrown by the failed compiler factory.
: */
: public CompilerFactory getFallbackFactory(TQueryOptions options, Exception e);
> I would prefer to keep fallback external to the compiler. I don't think the
Down the road, I was thinking that eventually we are going to treat Calcite as the default compiler and the Impala planner as the legacy. In addition to this, we might have various options doing different types of fallback. Heck, it might even return a different version of the Calcite factory.
Given that each compiler is going to do its own thing and have various options, I thought it would be nice to let the compiler return the fallback factory.
If you want me to keep things simple for now, I guess I'm ok with it. Not gonna fight hard on this one, but hardcoding the legacy factory as the fallback is gonna change in the future.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Tue, 04 Mar 2025 04:57:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 1: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11284/
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jan 2025 01:26:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17920/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 7
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Thu, 09 Jan 2025 15:23:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17912/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 6
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2025 01:39:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#20).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
18 files changed, 546 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/20
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 20
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#16).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
There is an additional "getFallbackFactory" method included in the
CompilerFactory interface. This method is meant to allow the CompilerFactory
to supply a "fallback" compiler to use if it cannot handle the sql
statement passed in. For the Impala implementation, there is no fallback
and will return null.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
18 files changed, 575 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/16
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 16
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17878/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 19:56:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 22:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11650/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 22
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 00:49:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#11).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
There is an additional "getFallbackFactory" method included in the
CompilerFactory interface. This method is meant to allow the CompilerFactory
to supply a "fallback" compiler to use if it cannot handle the sql
statement passed in. For the Impala implementation, there is no fallback
and will return null.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
19 files changed, 778 insertions(+), 251 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/11
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 24:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11653/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 24
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 14:57:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 18:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@549
PS17, Line 549:
> Nit: stray whitespace
Done
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@45
PS17, Line 45: // the wrapped sql statement object. Note, this is not final because Impala
: // allows the statement to be rewritten at analysis time (which is kinda
: // hacky)
: private final StatementBase stmt_ ;
> We've got two choices here:
Heh, yeah, I don't like any of them either, but you're right, we gotta do something to prevent bugs in the future
How about a third option, very similar to #2, but at least it will help with debugging:
Keep it final, but add a different non-final variable called "invalidated_"
Set this after analysisResult. And then in the getTopLevelNode(), we can throw an exception if true.
This will at least make it explicit while it's failing as opposed to just returning a null.
Or is that too complicated?
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@54
PS11, Line 54: public SingleNodePlannerIntf createSingleNodePlanner(PlannerContext ctx)
: throws ImpalaException;
:
: /**
: * Return a fallback compiler factory given a planner context allowing the
: * implementation to choose if it should fallback based on a query option or
> For now, let's keep the code simple. Down the road, we'll have better infor
I kinda disagree with this one, but I'm not gonna argue about it. Thought this was a pretty slick way of doing it. The fallback wasn't external...it just passed back the CompilerFactory to use if it fails.
However, I'm not sure what you're picturing here then as to how to trigger the fallback. We can't have any mention of Calcite in the main code (except for the class name through reflection).
Were you picturing just having a boolean variable that says "allowsFallback" and then use the internal compilerFactory if it does? This code almost does the same thing...it just returns the internal compiler factory rather than hardcoding it.
Or were you picturing the getTExecRequest to be an abstract method somewhere within the compilerFactory and the code is in the implementation? We'd have to pass Frontend into the method in that case, which I'm not sure I like. And that also makes fallback internal within the compiler.
Or maybe I'm just missing something obvious.
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/service/Frontend.java@2952
PS17, Line 2952: the
> We're using the ParsedStatement from the parser, but the AnalysisResult cou
This needs fixing, done
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 18
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Mar 2025 23:39:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 18:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18614/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 18
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Mar 2025 00:06:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 21: Code-Review+1
(3 comments)
This is looking good to me
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@497
PS21, Line 497: //XXX: returnsAtMostOneRow should be in interface
Let's file a JIRA for this. It's low priority as this seems like an edge case for result spooling.
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@22
PS21, Line 22: import org.apache.impala.analysis.AnalysisContext.AnalysisDriverImpl;
: import org.apache.impala.analysis.AnalysisContext.AnalysisResult;
: import org.apache.impala.analysis.AnalysisDriver;
: import org.apache.impala.analysis.Parser;
: import org.apache.impala.analysis.QueryStmt;
: import org.apache.impala.analysis.TableName;
: import org.apache.impala.analysis.StatementBase;
: import org.apache.impala.analysis.StmtMetadataLoader;
: import org.apache.impala.authorization.AuthorizationFactory;
: import org.apache.impala.common.ImpalaException;
: import org.apache.impala.thrift.TQueryCtx;
: import org.apache.impala.thrift.TQueryOptions;
: import org.apache.impala.thrift.TResultSetMetadata;
: import org.apache.impala.thrift.TStmtType;
:
: import static org.apache.impala.analysis.ToSqlOptions.SHOW_IMPLICIT_CASTS;
Nit: Let's take a pass at cleaning up these imports (e.g. AnalysisDriverImpl, AnalysisResult, AuthorizationFactory, TResultSetMetadata, TStmtType, SHOW_IMPLICT_CASTS)
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/service/Frontend.java@3522
PS21, Line 3522: private CompilerFactory getCompilerFactory(PlanCtx ctx) {
: return new CompilerFactoryImpl();
: }
Nit: I think this is unused now
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 21
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Mar 2025 20:56:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 24: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 24
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 20:36:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 22:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@91
PS22, Line 91: legacy
nit: in CompilerFactory the Impala planner is called OriginalPlanner. Let's use the same terminology here instead of legacy to be consistent.
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@93
PS22, Line 93: legacy
nit: see above
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 22
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 01:05:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17909/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 5
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2025 01:19:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 1:
(17 comments)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@590
PS1, Line 590:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@616
PS1, Line 616: // accurate count of the total number of expressions. The first analyze() call is not
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@617
PS1, Line 617: // very expensive (~seconds) even for large statements. The limit on the total length
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@620
PS1, Line 620: // expensive with large expression trees. For example, a SQL that takes a few seconds
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@623
PS1, Line 623:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@635
PS1, Line 635:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@639
PS1, Line 639: shouldReAnalyze = analysisResult_.stmt_.resolveTableMask(analysisResult_.analyzer_);
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@644
PS1, Line 644: // To be consistent with Hive, privilege requests introduced by the masking views
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@674
PS1, Line 674:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@675
PS1, Line 675: // For SetOperationStmt we must replace the query statement with the rewritten version
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@688
PS1, Line 688:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@693
PS1, Line 693:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@704
PS1, Line 704:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@745
PS1, Line 745:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/service/Frontend.java@472
PS1, Line 472: "org.apache.impala.calcite.service.CalciteParsedStatementCreator").newInstance();
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/main/java/org/apache/impala/service/Frontend.java@481
PS1, Line 481: }
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/22306/1/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@284
PS1, Line 284: AnalysisDriverImpl analysisDriver = (AnalysisDriverImpl) stmt.getAnalysisDriver(ctx);
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 19:32:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11284/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 19:30:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 5:
(8 comments)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@611
PS5, Line 611: // accurate count of the total number of expressions. The first analyze() call is not
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@612
PS5, Line 612: // very expensive (~seconds) even for large statements. The limit on the total length
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@615
PS5, Line 615: // expensive with large expression trees. For example, a SQL that takes a few seconds
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@640
PS5, Line 640: // To be consistent with Hive, privilege requests introduced by the masking views
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@671
PS5, Line 671: // For SetOperationStmt we must replace the query statement with the rewritten version
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java:
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@45
PS5, Line 45:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/main/java/org/apache/impala/service/Frontend.java@472
PS5, Line 472: "org.apache.impala.calcite.service.CalciteParsedStatementCreator").newInstance();
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/22306/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@284
PS5, Line 284: AnalysisDriverImpl analysisDriver = (AnalysisDriverImpl) stmt.getAnalysisDriver(ctx);
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 5
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2025 00:55:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: do not review
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#6).
Change subject: WIP: do not review
......................................................................
WIP: do not review
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreator.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreatorImpl.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
22 files changed, 683 insertions(+), 231 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/6
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 6
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 20:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18639/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 20
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Mar 2025 20:49:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 22: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11650/
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 22
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 06:21:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 2:
(17 comments)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@594
PS2, Line 594:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@620
PS2, Line 620: // accurate count of the total number of expressions. The first analyze() call is not
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@621
PS2, Line 621: // very expensive (~seconds) even for large statements. The limit on the total length
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@624
PS2, Line 624: // expensive with large expression trees. For example, a SQL that takes a few seconds
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@627
PS2, Line 627:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@639
PS2, Line 639:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@643
PS2, Line 643: shouldReAnalyze = analysisResult_.stmt_.resolveTableMask(analysisResult_.analyzer_);
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@648
PS2, Line 648: // To be consistent with Hive, privilege requests introduced by the masking views
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@678
PS2, Line 678:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@679
PS2, Line 679: // For SetOperationStmt we must replace the query statement with the rewritten version
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@692
PS2, Line 692:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@697
PS2, Line 697:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@708
PS2, Line 708:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@749
PS2, Line 749:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/service/Frontend.java@472
PS2, Line 472: "org.apache.impala.calcite.service.CalciteParsedStatementCreator").newInstance();
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/main/java/org/apache/impala/service/Frontend.java@481
PS2, Line 481: }
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/22306/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@285
PS2, Line 285: AnalysisDriverImpl analysisDriver = (AnalysisDriverImpl) stmt.getAnalysisDriver(ctx);
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jan 2025 23:12:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11290/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jan 2025 23:10:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18520/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Mar 2025 18:39:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 16:
(1 comment)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22306/16/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/16/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@549
PS16, Line 549:
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 16
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Mar 2025 14:25:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 21:
(5 comments)
Ok, think I got all of this done.
Wanted to put in here as a last comment a potential future change though.
In the original patch, I put in a generic way of handling fallback. After discussion, I was pointed out a flaw in the design. The getFallbackFactory should not have been in the interface since there could theoretically be multiple planners. The framework should have been in control of that.
But I think I went overboard in moving things out of the Calcite planner for the record. If a statement is a non-select (e.g. create table), the code to fallback is in the framework. THis is really specific to the Calcite planner, and the logic should be in there, not in the framework. The framework really shouldn't be aware of the Calcite planner, imo.
Perhaps this is future work, but this has gotten a +2 now, so I will be committing this soon.
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@91
PS21, Line 91: private final ParsedStatement parsedStmt_;
> nit: it would be good to add a comment here about why both ParsedStatement
Done
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@106
PS21, Line 106: : null;
> Having a null value for stmt_ is not a good state as a lot of code derefere
I put it in the comment, but this will be null for the Calcite planner. The StatementBase is legacy planner specific.
As you mentioned, I kept this for convenience. Just looking at lines 111 and below, the code review would have been bigger (well maybe not that much bigger) if I dereferenced parsedStmt all the time, so I figured this would be better.
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@497
PS21, Line 497: //XXX: returnsAtMostOneRow should be in interface
> Let's file a JIRA for this. It's low priority as this seems like an edge ca
Done
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@22
PS21, Line 22: import org.apache.impala.analysis.AnalysisContext.AnalysisDriverImpl;
: import org.apache.impala.analysis.AnalysisContext.AnalysisResult;
: import org.apache.impala.analysis.AnalysisDriver;
: import org.apache.impala.analysis.Parser;
: import org.apache.impala.analysis.QueryStmt;
: import org.apache.impala.analysis.TableName;
: import org.apache.impala.analysis.StatementBase;
: import org.apache.impala.analysis.StmtMetadataLoader;
: import org.apache.impala.authorization.AuthorizationFactory;
: import org.apache.impala.common.ImpalaException;
: import org.apache.impala.thrift.TQueryCtx;
: import org.apache.impala.thrift.TQueryOptions;
: import org.apache.impala.thrift.TResultSetMetadata;
: import org.apache.impala.thrift.TStmtType;
:
: import static org.apache.impala.analysis.ToSqlOptions.SHOW_IMPLICIT_CASTS;
> Nit: Let's take a pass at cleaning up these imports (e.g. AnalysisDriverImp
Done
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/service/Frontend.java@3522
PS21, Line 3522: private CompilerFactory getCompilerFactory(PlanCtx ctx) {
: return new CompilerFactoryImpl();
: }
> Nit: I think this is unused now
Done
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 21
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 00:46:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 23: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 23
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 01:27:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 24:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18712/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 24
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 14:13:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17894/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jan 2025 23:36:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18526/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 14
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Mar 2025 15:59:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 24: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 24
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 20:44:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@54
PS11, Line 54: /**
: * Return a fallback compiler factory given a planner context allowing the
: * implementation to choose if it should fallback based on a query option or
: * the type of exception thrown by the failed compiler factory.
: */
: public CompilerFactory getFallbackFactory(TQueryOptions options, Exception e);
> I kinda disagree with this one, but I'm not gonna argue about it. Thought
I would do something like this (which might fit better in the Calcite add-on patch rather than the original patch):
private TExecRequest getTExecRequestWithFallback(
PlanCtx planCtx, EventSequence timeline) throws ImpalaException {
TExecRequest request = null;
if (condition_to_try_calcite) {
try {
// getCalciteCompilerFactory() has a return type of CompilerFactory
request = getTExecRequest(getCalciteCompilerFactory(), planCtx, timeline);
addPlannerToProfile(CALCITE);
} catch (Exception e) {
// In the testing mode, this would run the original parser and see if this is a
// QueryStmt. In production, this just returns true.
// This could pass in the exception too, but I don't think it needs it right now.
if (!shouldFallbackToRegularPlanner(planCtx)) {
throw e;
}
LOG.info("Calcite planner failed: " + e);
timeline.markEvent("Failing over from Calcite planner");
}
}
if (request == null) {
addPlannerToProfile(PLANNER);
result = getTExecRequest(getRegularCompilerFactory(), planCtx, timeline);
}
return result;
}
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Mar 2025 18:01:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 19:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18625/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 19
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Mar 2025 02:25:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 11:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@98
PS11, Line 98: // Hack: stmt_ and analyzer_ are not final because of the "rewrite"
Drop "Hack:"
I would use "because rewrites overwrite these fields"
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@575
PS11, Line 575: public static class AnalysisDriverImpl implements AnalysisDriver {
The old code had a AnalysisContext that produces an AnalysisResult. It wasn't perfect, but it had a division of responsibilities. The AnalysisDriver borrows a mixture responsibilities from the other two, and the resulting design doesn't really have a clear set of responsibilities for any of the classes.
AnalysisDriver is a field on AnalysisResult to convey extra information out of analysis, but it also creates the Analyzer and implements analyze(), which fills in its parent class.
It functions, but it is a weird design. I want something where the classes have a clearer set of responsibilities. I don't place any importance on overriding a single class rather than two classes.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@586
PS11, Line 586: ctx_ = analysisContext;
Style-wise, we only use the analysisResult_ field on ctx_ and we use it in many locations. I think we should have an analysisResult_ field on this so that we don't need to do ctx_.analysisResult_ every time.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java@30
PS11, Line 30: raferenced
Nit: referenced
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@45
PS11, Line 45: // the wrapped sql statement object. Note, this is not final because Impala
: // allows the statement to be rewritten at analysis time (which is kinda
: // hacky)
: private StatementBase stmt_ ;
If I'm understanding this right, we don't use setTopLevelNode() anywhere, so we can remove that and make this final.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@78
PS11, Line 78: // set the wrapped statement on rewrite.
: public void setTopLevelNode(StatementBase stmt) {
: stmt_ = stmt;
: }
It looks like we don't use this anywhere.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@28
PS11, Line 28: interfaces
Nit: interface
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@31
PS11, Line 31: parser and the
: * Calcite parser.
Nit: Can we use a word other than "parser"?
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@36
PS11, Line 36: /**
: * Create a ParsedStatement class which does the parsing of the query.
: */
: public ParsedStatement createParsedStatement(TQueryCtx queryCtx) throws ImpalaException;
Down the road, frontend tests are going to be using this rather than creating the classes manually. So, there will be multiple signatures for this function (which might motivate an AbstractCompilerFactory).
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@54
PS11, Line 54: /**
: * Return a fallback compiler factory given a planner context allowing the
: * implementation to choose if it should fallback based on a query option or
: * the type of exception thrown by the failed compiler factory.
: */
: public CompilerFactory getFallbackFactory(TQueryOptions options, Exception e);
I would prefer to keep fallback external to the compiler. I don't think the compiler has any extra information that helps the fallback decision. The compiler should be simple: Try to compile the statement, fail if it can't.
Fallback is very simple today and can be expressed in 30 lines of code. I'd be reluctant to make it an abstract algorithm.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
File fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@35
PS11, Line 35: * Factory implementation which creates Compiler implementation classes.
Let's be clear that this is for the original planner.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@37
PS11, Line 37: CompilerFactoryImpl
If the Calcite version is going to be called CalciteCompilerFactory, should this be called OriginalCompilerFactory or RegularCompilerFactory?
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java
File fe/src/test/java/org/apache/impala/common/FrontendFixture.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@359
PS11, Line 359: ParsedStatement parsedStmt = new ParsedStatementImpl(stmt);
I think we'll end up using a compiler factory to create this. This function may end up having a signature that passes in the compiler factory. There are times when we want to parse using the compiler under test, but there are also times where it must always be the regular compiler (e.g. when creating a view).
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@370
PS11, Line 370: CompilerFactory compilerFactory = new CompilerFactoryImpl();
I think the FrontendFixture will have the concept of the compiler under test. This would use the compiler under test rather than creating its own or a creating a parsed statement directly. The ParsedStatement needs to be produced by the same compiler factory as we use for analyzeAndAuthorize.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Tue, 04 Mar 2025 03:58:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 21: Code-Review+1
(2 comments)
This generally lgtm and it has already gone through reviews. Just a couple of comments. Doing a +1 for now.
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@91
PS21, Line 91: private final ParsedStatement parsedStmt_;
nit: it would be good to add a comment here about why both ParsedStatement and StatementBase are needed. The latter is derivable from the former but it appears to be more as a convenience to minimize code changes.
http://gerrit.cloudera.org:8080/#/c/22306/21/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@106
PS21, Line 106: : null;
Having a null value for stmt_ is not a good state as a lot of code dereferences this. Did you have this for specific reason ? In the Parser.java, the parse() method either returns a StatementBase object or it throws an exception. If nulls are not expected, it is best to add a precondition check.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 21
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Mar 2025 19:01:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#22).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
17 files changed, 506 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/22
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 22
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 22:
(1 comment)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@94
PS22, Line 94: // but in its own variable for convenience. For the Calcite planner, this will be null.
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 22
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 00:44:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 21:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18646/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 21
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Mar 2025 06:29:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#10).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
There is an additional "getFallbackFactory" method included in the
CompilerFactory interface. This method is meant to allow the CompilerFactory
to supply a "fallback" compiler to use if it cannot handle the sql
statement passed in. For the Impala implementation, there is no fallback
and will return null.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
19 files changed, 791 insertions(+), 251 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/10
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 10
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18371/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Tue, 25 Feb 2025 18:56:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 16:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18549/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 16
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Mar 2025 14:51:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#2).
Change subject: WIP: do not review
......................................................................
WIP: do not review
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreator.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreatorImpl.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
22 files changed, 724 insertions(+), 235 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/2
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 23:
Carry forward Joe's +1 and bumping mine.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 23
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 01:32:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Reviewed-on: http://gerrit.cloudera.org:8080/22306
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Aman Sinha <am...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
17 files changed, 507 insertions(+), 124 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Aman Sinha: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 25
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 22:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@91
PS22, Line 91: legacy
> nit: in CompilerFactory the Impala planner is called OriginalPlanner. Let's
Done
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@93
PS22, Line 93: legacy
> nit: see above
Done
http://gerrit.cloudera.org:8080/#/c/22306/22/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@94
PS22, Line 94: // but in its own variable for convenience. For the Calcite planner, this will be null.
> line too long (91 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 22
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 01:11:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@575
PS11, Line 575: public static class AnalysisDriverImpl implements AnalysisDriver {
> I'm not sure I quite understand what the division of responsibilities is.
I thought of 4 different designs I could go with in which I have a clear ranking in my mind. Not sure if this is the place to write it or if there is a better place to put this info. I already wrote a wall of text here.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Tue, 04 Mar 2025 13:50:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 17:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@549
PS17, Line 549:
Nit: stray whitespace
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@45
PS17, Line 45: // the wrapped sql statement object. Note, this is not final because Impala
: // allows the statement to be rewritten at analysis time (which is kinda
: // hacky)
: private final StatementBase stmt_ ;
We've got two choices here:
1. ParsedStatementImpl can allow modifications to stmt_. This means that the original ParsedStatement from the parser is same as the one on AnalysisResult. They point to the same object, but it can change during analysis.
2. ParsedStatementImpl can't be modified. AnalysisResult creates a new ParsedStatementImpl when it rewrites a query and has a new StatementBase. There are two different ParsedStatements that could be slightly different. One is directly from the parser. The other is on the AnalysisResult with the rewritten statement.
Right now, we're doing #2, so we need to be careful about which ParsedStatement we are using. Maybe #1 is easier to reason about as it doesn't end up with two different objects. They both have downsides, so I'm not sure which is better.
If we go with #1, then this class will have some way to modify stmt_ and this will not be final.
If we go with #2, then we can drop the comment about this not being final. We might want to take additional steps to avoid problems with the two different ParsedStatement objects. e.g. Null out the ParsedStatement from the parser once we get the AnalysisResult back, forcing subsequent code to use the ParsedStatement from the AnalysisResult.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@54
PS11, Line 54: public SingleNodePlannerIntf createSingleNodePlanner(PlannerContext ctx)
: throws ImpalaException;
:
: /**
: * Return a fallback compiler factory given a planner context allowing the
: * implementation to choose if it should fallback based on a query option or
> Down the road, I was thinking that eventually we are going to treat Calcite
For now, let's keep the code simple. Down the road, we'll have better information about what exactly we want to do for fallback and why. If we need something like this in the future, we will introduce it.
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/17/fe/src/main/java/org/apache/impala/service/Frontend.java@2952
PS17, Line 2952: parsedStmt
We're using the ParsedStatement from the parser, but the AnalysisResult could have a different ParsedStatement that was rewritten. I don't know if that makes a difference (hopefully not).
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 17
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Mar 2025 22:00:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#18).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
There is an additional "getFallbackFactory" method included in the
CompilerFactory interface. This method is meant to allow the CompilerFactory
to supply a "fallback" compiler to use if it cannot handle the sql
statement passed in. For the Impala implementation, there is no fallback
and will return null.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
18 files changed, 578 insertions(+), 125 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/18
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 18
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 13:
(12 comments)
This should still be reviewed after https://gerrit.cloudera.org/#/c/22591/
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@575
PS11, Line 575: return analyzer_;
> I thought of 4 different designs I could go with in which I have a clear ra
As mentioned in the general comment, this design is changed in IMPALA-13481
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@586
PS11, Line 586: y {
> Style-wise, we only use the analysisResult_ field on ctx_ and we use it in
Same as comment above
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java@30
PS11, Line 30: referenced
> Nit: referenced
Done
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
File fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@45
PS11, Line 45: // the wrapped sql statement object. Note, this is not final because Impala
: // allows the statement to be rewritten at analysis time (which is kinda
: // hacky)
: private final StatementBase s
> If I'm understanding this right, we don't use setTopLevelNode() anywhere, s
Done
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java@78
PS11, Line 78: @Override
: public boolean isExplain() {
: return stmt_.isExplain();
: }
> It looks like we don't use this anywhere.
Done
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@28
PS11, Line 28: interface
> Nit: interface
Done
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@31
PS11, Line 31: planner and the
: * Calcite planner
> Nit: Can we use a word other than "parser"?
Done
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@36
PS11, Line 36: public interface CompilerFactory {
:
: /**
: * Create a ParsedStatement class which does the parsing of the query.
> Down the road, frontend tests are going to be using this rather than creati
Ack. We can discuss further on that commit, I don't mind re-changing the API. I think it's possible without changing it, but I guess it depends on how much restructuring we want to do.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
File fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@35
PS11, Line 35:
> Let's be clear that this is for the original planner.
Ok, changed comment. Looking at the comment below, maybe we should call it "Original". Idk, even that seems off.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@37
PS11, Line 37: plementation of the
> If the Calcite version is going to be called CalciteCompilerFactory, should
I struggled with the name here. Regular doesn't seem right to me. Original maybe works? I'm not sure. I liked going with just "Impl" because it's the standard implementation.
But I'm definitely open to changing this.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java
File fe/src/test/java/org/apache/impala/common/FrontendFixture.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@359
PS11, Line 359: ParsedStatement parsedStmt = new ParsedStatementImpl(stmt);
> I think we'll end up using a compiler factory to create this. This function
Ack. Let's discuss this further when that commit comes in.
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@370
PS11, Line 370: CompilerFactory compilerFactory = new CompilerFactoryImpl();
> I think the FrontendFixture will have the concept of the compiler under tes
Ack. Let's discuss this further when that commit comes in.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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, 08 Mar 2025 18:12:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#7).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- ParsedStatementCreator: This factory hook has one method "create" which
will create a ParsedStatement interface object. The idea is to
introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the ParsedStatementCreatorImpl object which
will instantiate a ParsedStatementImpl (next bullet point)
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method. The "getAnalysisDriver" method returns an
AnalysisDriver object which is the next step in the compilation process.
The implementation for the Impala planner can be found in the
ParsedStatementImpl class.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object. The "createSingleNodePlanner" serves as a method
to fetch the next interface hook, the "SingleNodePlannerIntf" object.
One hack note here: If this were refactored properly, there should be
no reason for the SingleNodePlannerIntf object to be stored in the
AnalysisResult. It has been placed there to keep this code review simple.
Some broader refactoring is needed to make this code cleaner.
- SingleNodePlannerIntf: The implementation of this interface will take
the AnalysisResult and return a PlanNode that can run on a single node
via the "createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects. Again, while
the hooks and the API could be made simpler, the decision for the
methods chosen in the API were to keep the code review as simple as
possible.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreator.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreatorImpl.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
18 files changed, 657 insertions(+), 230 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/7
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 7
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] WIP: do not review
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#5).
Change subject: WIP: do not review
......................................................................
WIP: do not review
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/Query.thrift
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreator.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementCreatorImpl.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
22 files changed, 682 insertions(+), 231 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/5
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 5
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 10:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/18323/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 10
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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, 21 Feb 2025 00:43:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 8:
I looked at the alternate implementation and I disagree with the design decision of subclassing AnalysisContext and AnalysisResult.
I feel that an alternative planning mechanism, should have an API that is as simple as possible. The top level of simplicity would be the number of classes defined within the API. To me, it makes more sense to have only one API class/interface for parsing (we do), one API class/interface for the Planner (we do), but also only one API class/interface for Analysis. The alternative proposal forces an API developer to derive from the 2 classes AnalysisContext and AnalysisResult.
The second issue I have with the alternative proposal (kinda similar to the first objection I have) is that I believe the API should be a pure interface, and then we should implement the interface with the legacy planner and the Calcite planner. An interface keeps both implementation with an equal footing and pushes shared code into a location which can be used by both implementations. The alternative proposal treats AnalysisContext and AnalysisPlanner as base classes. Some of the code is then used in the base class and some of the methods are overridden. I find this confusing when doing code inspection on a new piece of code. I have to check to look at the default legacy implementation in the base class but then look in the derived Calcite class to see if that specific method was overridden.
The one advantage I see for using the alternative approach is that anyone familiar with the current code might feel more comfortable with keeping the familiar classes? And it's less disruptive for someone who knows the legacy code? I don't feel this is enough of a reason to go with that approach since the whole Analysis module should probably be rewritten (e.g. AnalysisContext is not really a context as defined by most code) and it's already confusing right now.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 8
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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, 13 Feb 2025 19:08:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#14).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
There is an additional "getFallbackFactory" method included in the
CompilerFactory interface. This method is meant to allow the CompilerFactory
to supply a "fallback" compiler to use if it cannot handle the sql
statement passed in. For the Impala implementation, there is no fallback
and will return null.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
19 files changed, 571 insertions(+), 119 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/14
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 14
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#19).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
18 files changed, 548 insertions(+), 125 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/19
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 19
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#24).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
17 files changed, 507 insertions(+), 124 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/24
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 24
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22306
to look at the new patch set (#23).
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
17 files changed, 507 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/23
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 23
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@575
PS11, Line 575: public static class AnalysisDriverImpl implements AnalysisDriver {
> The old code had a AnalysisContext that produces an AnalysisResult. It wasn
I'm not sure I quite understand what the division of responsibilities is. I guess I can try...
AnalysisContext isn't really a context. It seems kinda like the Driver?
AnalysisResult also isn't really a Result class since it contains the Analyzer? Which I suppose contains results, but then it contains results within the Analyzer and outside the Analyzer?
And then we make calls off of the analysisResult stmt like analysisResult_.stmt_.analyze(analysisResult_.analyzer_)? Which a) accesses a member variable of AnalysisResult, b) calls a method off of it(???), and c) passes in a third object which is a member variable that is mutated(???) Mutating a variable like this while containing other mutations is a big no-no for me and I'm having a hard time wrapping my head around this without a big rewrite.
So the Context class isn't really a Context class and the Result class isn't really a Result class.
Is it a wrapper class and an implementor/result-ish class?
So I had 2 goals in mind with any design, and I have to stay true to these ideals for anything I write. I thought I did that here, but I can try again to see if I can come up with something we both find palatable.
Those 2 goals are:
1) Keep the changes somewhat minimal. Any code review is going to be difficult. The more stuff I move around, the more prone to error this is and the more difficult it will be to review. I think I accomplished that here...while the review has 700+ lines, half of the changes within AnalysisContext which are just adding "ctx_" and adding some member variables. I kept logic flow changes to a minimum, I think.
2) The design has to have pure interfaces. Shared logic should be in a shared class, but the shared class should not contain any method that is implementation specific. This doesn't fly for me because it's the type of class that is very hard to debug by code inspection since one has to ignore the base class method and specifically remember (or read a comment) that it is overridden in a derived class.
I think I accomplished my goals here. I don't really think I made anything worse by creating a wrapper around the code that exists. IMO, we should create a Jira to do a general rewrite rather than try to figure out a compromise API using 2 classes which is why I chose this implementation. And I think that general rewrite is too big of a change for what is trying to be accomplished in this code review.
And 2) is kinda my problem here. Some of AnalysisContext is implementation specific. Some of it is not. I can move the implementation specific code into a separate interface/derived class? But then the code review becomes a little tougher. Or I can keep it all in one file and immediately break it up into a different file in a second code review? I can see what that looks like, I suppose, but while you don't place any importance on 2 separate analysis API classes, I kinda find it awkward.
Even worse for me for case 2) is that I don't think I can create a CalciteAnalysisResult class (as proposed in another PR) and feel good about it. Because of the nature of accessing member variables directly, it might be impossible to create an interface...it might have to be a derived class. And I don't think I feel comfortable writing that. But I can try and see what it looks like.
Anyway, I'll look at this a little more but I wanted to share my initial thoughts before I dig in and try to redesign something we both can agree upon.
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 11
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Tue, 04 Mar 2025 06:43:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
IMPALA-13653: Create hooks for Calcite planner in Frontend
This commit creates hooks into the Impala planner that will
call into the Calcite planner.
There is one factory hook and 3 compilation hooks introduced to
allow this to happen:
- CompilerFactory : This factory hook has methods to create the
implementations for the three interfaces described below. The idea is
to introduce a query option that allows the Calcite objects to be called
instead of the Impala planner. In this iteration, the only
implementation called is the Impala specific implementations.
- ParsedStatement: The implementation of this interface will parse
the SQL query into an AST in its constructor. The implementation will
be a wrapper for the AST which can be accessed through the "getTopLevelNode"
method. For the current planner, the "StatementBase" object will be
accessible via this method.
- AnalysisDriver: The implementation of this interface will analyze the
parsed AST via the "analyze" method. To make the code review as easy
as possible, the AnalyzeDriverImpl implementation has been placed inside
the AnalysisContext object.
- SingleNodePlannerIntf: The implementation of this interface will
return a PlanNode that can run on a single node via the
"createSingleNodePlan" method. Various other methods in this
interface allow access to data structures that are needed for the final
plan. These objects are related to the output Expr objects.
There is an additional "getFallbackFactory" method included in the
CompilerFactory interface. This method is meant to allow the CompilerFactory
to supply a "fallback" compiler to use if it cannot handle the sql
statement passed in. For the Impala implementation, there is no fallback
and will return null.
Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatement.java
A fe/src/main/java/org/apache/impala/analysis/ParsedStatementImpl.java
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
A fe/src/main/java/org/apache/impala/common/FallbackException.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
A fe/src/main/java/org/apache/impala/service/CompilerFactory.java
A fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
19 files changed, 570 insertions(+), 119 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/22306/13
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 13
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 10:
(4 comments)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22306/10/fe/src/main/java/org/apache/impala/service/CompilerFactory.java
File fe/src/main/java/org/apache/impala/service/CompilerFactory.java:
http://gerrit.cloudera.org:8080/#/c/22306/10/fe/src/main/java/org/apache/impala/service/CompilerFactory.java@44
PS10, Line 44: public AnalysisDriver createAnalysisDriver(AnalysisContext ctx) throws AnalysisException;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/10/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java
File fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java:
http://gerrit.cloudera.org:8080/#/c/22306/10/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@41
PS10, Line 41: public ParsedStatement createParsedStatement(TQueryCtx queryCtx) throws ImpalaException {
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/10/fe/src/main/java/org/apache/impala/service/CompilerFactoryImpl.java@45
PS10, Line 45: public AnalysisDriver createAnalysisDriver(AnalysisContext ctx) throws AnalysisException {
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/10/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/22306/10/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@356
PS10, Line 356: ParsedStatement parsedStmt = compilerFactory.createParsedStatement(ctx.getQueryCtx());
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 10
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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, 21 Feb 2025 00:26:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13653: Create hooks for Calcite planner in Frontend
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: IMPALA-13653: Create hooks for Calcite planner in Frontend
......................................................................
Patch Set 23:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18702/ : 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/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 23
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Mar 2025 01:35:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jan 2025 04:40:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: do not review
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22306 )
Change subject: WIP: do not review
......................................................................
Patch Set 6:
(8 comments)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@611
PS6, Line 611: // accurate count of the total number of expressions. The first analyze() call is not
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@612
PS6, Line 612: // very expensive (~seconds) even for large statements. The limit on the total length
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@615
PS6, Line 615: // expensive with large expression trees. For example, a SQL that takes a few seconds
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@640
PS6, Line 640: // To be consistent with Hive, privilege requests introduced by the masking views
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@671
PS6, Line 671: // For SetOperationStmt we must replace the query statement with the rewritten version
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java:
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@45
PS6, Line 45:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/main/java/org/apache/impala/service/Frontend.java@472
PS6, Line 472: "org.apache.impala.calcite.service.CalciteParsedStatementCreator").newInstance();
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/22306/6/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@284
PS6, Line 284: AnalysisDriverImpl analysisDriver = (AnalysisDriverImpl) stmt.getAnalysisDriver(ctx);
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/22306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978aa48b160984ee5d36244cd915940f838d141d
Gerrit-Change-Number: 22306
Gerrit-PatchSet: 6
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Jan 2025 01:14:16 +0000
Gerrit-HasComments: Yes