You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2024/02/27 17:57:55 UTC
[Impala-ASF-CR] IMPALA:12847: Expose computeScanRangeLocations and computeStats
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21077
Change subject: IMPALA:12847: Expose computeScanRangeLocations and computeStats
......................................................................
IMPALA:12847: Expose computeScanRangeLocations and computeStats
After IMPALA-12631, HdfsScanNode.computeScanRangeLocations() need check
if countStarSlot_ is null or not to schedule footer range only.
HdfsScanNode.computeScanRangeLocations() is called along with
HdfsScanNode.init() call.
An external frontend that has its own count star slot analysis and
initialization will need to recompute scan range assignment and stats
after HdfsScanNode.init(). Therefore, computeScanRangeLocations() and
computeStats() should be made idempotent after init() and exposed to
subclasses.
This patch decouple countStarSlot_ initialization from
computeScanRangeLocations() and raise access level of
computeScanRangeLocations() from private to protected.
Testing:
- Pass core tests.
Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
---
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
1 file changed, 81 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/21077/1
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 07:06:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 12:47:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 08:09:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@218
PS2, Line 218: if
> nit: 'if' instead of 'is'
Done
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@221
PS2, Line 221: protect
> Can we make this protected to be consistent with partitions_ member veriab
Done
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@431
PS2, Line 431: getSampledOrRawParti
> It would be preferable to name this method such that it reflects it can ret
Done
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1161
PS2, Line 1161: If 'sampledPartitions_' is not
> Since this method is no longer checking sampledParams_ itself but relying o
Done
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 02:21:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
IMPALA-12847: Expose computeScanRangeLocations and computeStats
After IMPALA-12631, HdfsScanNode.computeScanRangeLocations() need check
if countStarSlot_ is null or not to schedule footer range only.
HdfsScanNode.computeScanRangeLocations() is called along with
HdfsScanNode.init() call.
An external frontend that has its own count star slot analysis and
initialization will need to recompute scan range assignment and stats
after HdfsScanNode.init(). Therefore, computeScanRangeLocations() and
computeStats() should be made idempotent after init() and exposed to
subclasses.
This patch decouple countStarSlot_ initialization from
computeScanRangeLocations() and raise access level of
computeScanRangeLocations() from private to protected.
Testing:
- Pass core tests.
Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Reviewed-on: http://gerrit.cloudera.org:8080/21077
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
1 file changed, 84 insertions(+), 50 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21077
to look at the new patch set (#3).
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
IMPALA-12847: Expose computeScanRangeLocations and computeStats
After IMPALA-12631, HdfsScanNode.computeScanRangeLocations() need check
if countStarSlot_ is null or not to schedule footer range only.
HdfsScanNode.computeScanRangeLocations() is called along with
HdfsScanNode.init() call.
An external frontend that has its own count star slot analysis and
initialization will need to recompute scan range assignment and stats
after HdfsScanNode.init(). Therefore, computeScanRangeLocations() and
computeStats() should be made idempotent after init() and exposed to
subclasses.
This patch decouple countStarSlot_ initialization from
computeScanRangeLocations() and raise access level of
computeScanRangeLocations() from private to protected.
Testing:
- Pass core tests.
Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
---
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
1 file changed, 84 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/21077/3
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 2: Code-Review+1
(4 comments)
Thanks for the patch. A few small comments but changes overall lgtm.
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@218
PS2, Line 218: is
nit: 'if' instead of 'is'
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@221
PS2, Line 221: private
Can we make this protected to be consistent with partitions_ member veriable.
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@431
PS2, Line 431: getSampledPartitions
It would be preferable to name this method such that it reflects it can return either sampled or raw partitions. The current naming gives a first impression of returning only sampled partitions.
How about getSampledOrRawPartitions().
http://gerrit.cloudera.org:8080/#/c/21077/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1161
PS2, Line 1161: If 'sampleParams_' is not null
Since this method is no longer checking sampledParams_ itself but relying on other method, you could modify the comment accordingly.
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 01:59:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/15346/ : 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/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 02:45:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10316/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Feb 2024 08:09:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12847: Expose computeScanRangeLocations and computeStats
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Fang-Yu Rao, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21077
to look at the new patch set (#2).
Change subject: IMPALA-12847: Expose computeScanRangeLocations and computeStats
......................................................................
IMPALA-12847: Expose computeScanRangeLocations and computeStats
After IMPALA-12631, HdfsScanNode.computeScanRangeLocations() need check
if countStarSlot_ is null or not to schedule footer range only.
HdfsScanNode.computeScanRangeLocations() is called along with
HdfsScanNode.init() call.
An external frontend that has its own count star slot analysis and
initialization will need to recompute scan range assignment and stats
after HdfsScanNode.init(). Therefore, computeScanRangeLocations() and
computeStats() should be made idempotent after init() and exposed to
subclasses.
This patch decouple countStarSlot_ initialization from
computeScanRangeLocations() and raise access level of
computeScanRangeLocations() from private to protected.
Testing:
- Pass core tests.
Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
---
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
1 file changed, 81 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/21077/2
--
To view, visit http://gerrit.cloudera.org:8080/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA:12847: Expose computeScanRangeLocations and computeStats
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21077 )
Change subject: IMPALA:12847: Expose computeScanRangeLocations and computeStats
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/15339/ : 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/21077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia621309c67455bb599f71bec9efc1f67fc085022
Gerrit-Change-Number: 21077
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@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: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Feb 2024 18:24:14 +0000
Gerrit-HasComments: No