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 2023/02/02 16:58:54 UTC
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Steve Carlin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19469
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
IMPALA-11895: Need accessor methods for third party extension
Changed the viewability of some methods needed for a third
party extension.
Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
---
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
4 files changed, 24 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/19469/1
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 2: Verified+1 Code-Review+2
Carry forward Kurt and mine +1 and bumping to +2. Also carry forward the Verified +1 since there hasn't been code change since the last run.
Will wait for a few hours before merging though in case other reviewers have further comment.
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Feb 2023 20:05:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Daniel Becker, Kurt Deschler, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19469
to look at the new patch set (#2).
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
IMPALA-11895: Need accessor methods for third party extension
Changed the visibility of some methods needed for a third
party extension.
The methods included in this are for the statements 'compute stats',
'create function', and 'refresh/invalidate'.
Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
---
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
4 files changed, 24 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/19469/2
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9018/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Feb 2023 22:29:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12296/ : 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/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Feb 2023 17:18:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
IMPALA-11895: Need accessor methods for third party extension
Changed the visibility of some methods needed for a third
party extension.
The methods included in this are for the statements 'compute stats',
'create function', and 'refresh/invalidate'.
Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Reviewed-on: http://gerrit.cloudera.org:8080/19469
Reviewed-by: Aman Sinha <am...@cloudera.com>
Tested-by: Aman Sinha <am...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
4 files changed, 24 insertions(+), 3 deletions(-)
Approvals:
Aman Sinha: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 3
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1:
One extra note which I should have included the first time.
Another potential solution would be to copy the PartitionSpec in the get method. In this case, it might work...unless some other object is a member of PartitionSpec in which case it would have to be a deepCopy (and PartitionSpec then has the responsibility to make sure it does the deep copy). I just don't like the "mixed message" architecture approach since this isn't done elsewhere and it is better to make PartitionSpec an immutable object.
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Feb 2023 13:38:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
> Patch Set 1:
>
> (1 comment)
>
> Can we be sure that we don't run into problems if we expose these methods/variables? Won't users be able to change the state in ways the code doesn't expect?
Probably @scarlin can respond to this but I only see accessors have been added or changed so none of them modify state.
Carry forward Kurt's +1. Can bump to +2 once comments are addressed.
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG@9
PS1, Line 9: viewability
Should this be 'visibility' instead of 'viewability' ?
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG@9
PS1, Line 9: Changed the viewability of some methods needed for a third
> It would be more informative to list the methods that have been changed.
Instead of individual method names, you could mention that the accessor methods are related to COMPUTE STATS, CREATE FUNCTION / TABLE, REFRESH commands.
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Sat, 11 Feb 2023 00:32:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 03:39:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 00:32:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1:
(1 comment)
Can we be sure that we don't run into problems if we expose these methods/variables? Won't users be able to change the state in ways the code doesn't expect?
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG@9
PS1, Line 9: Changed the viewability of some methods needed for a third
It would be more informative to list the methods that have been changed.
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 12:24:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Steve Carlin (Code Review)" <ge...@cloudera.org>.
Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 1:
(3 comments)
So to the general question of "can we be sure that we don't run into problems if we expose these methods/variables?"
Not gonna give the greatest of answers here and it's a bit painful for me...
I think you might be referring to something like "getPartitionSpec" which returns a PartitionSpec object? Theoretically, a coder could change the internals of PartitionSpec since the variables are not declared as "final".
First: I'm not doing that in my third party code. Not that this answers the general issue, but I do need it for access, and my code will be safe.
Second: This is really a huuuuuge problem within the whole Impala codebase. The PartitionSpec internal variables get modified via the "analyze" command. This is a faulty design for the reason you're worried about (if that's what you are worried about). Perhaps a better design would have been to have an "AnalyzedPartitionSpec" or something like that? And, of course, we should declare all internal members as final.
Regardless...this is pervasive within the Impala code. I fixed a bug a month or two ago because of the planner code grabbed a "getChildExpr" from Expr and Expr changed the internals even after it was analyzed.
If I were to fix this the right way, it would be a much bigger effort at this point.
So I think we have to accept that there is an implicit contract that the caller of getPartitionSpec() is not going to mess around with the internals based on all the other code that exists that is similar to this. It fits in with the current design. This type of code is going to have to be caught at code review time. Not ideal and I don't like this at all, but I think this is the current way of dealing with issues like this.
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG@9
PS1, Line 9: viewability
> Should this be 'visibility' instead of 'viewability' ?
Done
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG@9
PS1, Line 9: Changed the viewability of some methods needed for a third
> Instead of individual method names, you could mention that the accessor met
Done
http://gerrit.cloudera.org:8080/#/c/19469/1//COMMIT_MSG@9
PS1, Line 9: Changed the viewability of some methods needed for a third
> It would be more informative to list the methods that have been changed.
I addressed the statements. I think putting the actual methods is a bit much since one only needs to look at the git diff.
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Sun, 12 Feb 2023 22:21:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11895: Need accessor methods for third party extension
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/19469 )
Change subject: IMPALA-11895: Need accessor methods for third party extension
......................................................................
Patch Set 2:
> Patch Set 1:
>
> One extra note which I should have included the first time.
>
> Another potential solution would be to copy the PartitionSpec in the get method. In this case, it might work...unless some other object is a member of PartitionSpec in which case it would have to be a deepCopy (and PartitionSpec then has the responsibility to make sure it does the deep copy). I just don't like the "mixed message" architecture approach since this isn't done elsewhere and it is better to make PartitionSpec an immutable object.
The first time analyze() is called, the partitionSpec_ is marked Immutable according to: https://github.com/apache/impala/blame/master/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java#L146
--
To view, visit http://gerrit.cloudera.org:8080/19469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaea15ef8a96c9509b9cd79df595868fd1db47e83
Gerrit-Change-Number: 19469
Gerrit-PatchSet: 2
Gerrit-Owner: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Feb 2023 19:59:51 +0000
Gerrit-HasComments: No