You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2022/11/30 11:45:02 UTC

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19289


Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................

IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Sequence container based file formats (SequenceFile, RCFile, Avro) have
a file header in each file that describes the metadata of the file, e.g.
codec, default values, etc. The header should be decoded before reading
the file content. The initial scanners will read the header and then
issue follow-up scan ranges for the file content. The decoded header
will be referenced by follow-up scanners.

Since IMPALA-9655, when MT_DOP > 1, the issued scan ranges could be
scheduled to other scan node instances. So the header resource should
live until all scan node instances close. Header objects are owned by
the object pool of the RuntimeState, which meets the requirement.

AvroFileHeader is special than other headers in that it references a
template tuple which contains the partition values and default values
for missing fields. The template tuple is initially owned by the header
scanner, then transferred to the scan node before the scanner closes.
However, when the scan node instance closes, the template tuple is
freed. Scanners of other scan node instances might still depend on it.
This could cause wrong results or crash the impalad.

When partition columns are used in the query, or when the underlying
avro files have missing fields and the table schema has default values
for them, the AvroFileHeader will have a non-null template tuple, which
could hit this bug when MT_DOP>1.

This patch fixes the bug by transferring the template tuple to
ScanRangeSharedState directly. The scan_node_pool of HdfsScanNodeBase is
also removed since it's only used to hold the template tuple (and
related buffers) of the avro header. Also no need to override
TransferToScanNodePool in HdfsScanNode since the original purpose is to
protect the pool by a lock, and now the method in ScanRangeSharedState
already has a lock.

Tests
 - Add missing test coverage for compute stats on avro tables. Note that
   MT_DOP=4 is set by default for compute stats.
 - Add the MT_DOP dimension for TestScannersAllTableFormats. Also add
   some queries that can reveal the bug in scanners.test. The ASAN build
   can easily crash by heap-use-after-free error with this fix.
 - Ran related tests locally.

Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
---
M be/src/exec/avro/hdfs-avro-scanner.cc
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/streaming-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/compute-stats-avro.test
M testdata/workloads/functional-query/queries/QueryTest/scanners.test
M tests/query_test/test_scanners.py
11 files changed, 227 insertions(+), 23 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/19289/1
-- 
To view, visit http://gerrit.cloudera.org:8080/19289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19289/4/testdata/workloads/functional-query/queries/QueryTest/compute-stats-avro.test
File testdata/workloads/functional-query/queries/QueryTest/compute-stats-avro.test:

http://gerrit.cloudera.org:8080/#/c/19289/4/testdata/workloads/functional-query/queries/QueryTest/compute-stats-avro.test@60
PS4, Line 60: set location '/test-warehouse/alltypes_avro_snap';
Need to prefix paths like this with $FILESYSTEM_PREFIX for them to work in Ozone tests.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Dec 2022 17:55:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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/19289 )

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................

IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Sequence container based file formats (SequenceFile, RCFile, Avro) have
a file header in each file that describes the metadata of the file, e.g.
codec, default values, etc. The header should be decoded before reading
the file content. The initial scanners will read the header and then
issue follow-up scan ranges for the file content. The decoded header
will be referenced by follow-up scanners.

Since IMPALA-9655, when MT_DOP > 1, the issued scan ranges could be
scheduled to other scan node instances. So the header resource should
live until all scan node instances close. Header objects are owned by
the object pool of the RuntimeState, which meets the requirement.

AvroFileHeader is special than other headers in that it references a
template tuple which contains the partition values and default values
for missing fields. The template tuple is initially owned by the header
scanner, then transferred to the scan node before the scanner closes.
However, when the scan node instance closes, the template tuple is
freed. Scanners of other scan node instances might still depend on it.
This could cause wrong results or crash the impalad.

When partition columns are used in the query, or when the underlying
avro files have missing fields and the table schema has default values
for them, the AvroFileHeader will have a non-null template tuple, which
could hit this bug when MT_DOP>1.

This patch fixes the bug by transferring the template tuple to
ScanRangeSharedState directly. The scan_node_pool of HdfsScanNodeBase is
also removed since it's only used to hold the template tuple (and
related buffers) of the avro header. Also no need to override
TransferToScanNodePool in HdfsScanNode since the original purpose is to
protect the pool by a lock, and now the method in ScanRangeSharedState
already has a lock.

Tests
 - Add missing test coverage for compute stats on avro tables. Note that
   MT_DOP=4 is set by default for compute stats.
 - Add the MT_DOP dimension for TestScannersAllTableFormats. Also add
   some queries that can reveal the bug in scanners.test. The ASAN build
   can easily crash by heap-use-after-free error without this fix.
 - Ran exhaustive tests.

Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Reviewed-on: http://gerrit.cloudera.org:8080/19289
Reviewed-by: Zoltan Borok-Nagy <bo...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/avro/hdfs-avro-scanner.cc
M be/src/exec/avro/hdfs-avro-scanner.h
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/streaming-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/compute-stats-avro.test
M testdata/workloads/functional-query/queries/QueryTest/scanners.test
M tests/query_test/test_scanners.py
12 files changed, 231 insertions(+), 25 deletions(-)

Approvals:
  Zoltan Borok-Nagy: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19289 )

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 3: Code-Review+2

Thanks for fixing this!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 12:06:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 1:

(3 comments)

Thanks for Zoltan's quick review!

http://gerrit.cloudera.org:8080/#/c/19289/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19289/1//COMMIT_MSG@47
PS1, Line 47: with
> without?
Oops, done


http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.cc
File be/src/exec/avro/hdfs-avro-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.cc@292
PS1, Line 292: no partition columns
> nit: and no default values?
Done. BTW, this method is used exactly when we write default values to "avro_header_->template_tuple". It could only be nullptr in the first call.


http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/hdfs-scan-node-base.cc@1305
PS1, Line 1305: metadata_lock_
> Currently 'metadata_lock_' is used to protect 'per_file_metadata_'.
Yeah, IIUC, HdfsScanPlanNode is different than HdfsScanNodeBase (HdfsScanNode/HdfsScanNodeMt) that it's owned by the FragmentState. HdfsScanPlanNode::Close() is called by FragmentState::ReleaseResources(). At that time, all the fragment instances have already been closed. So no issues in HdfsScanPlanNode::Close().

The other two accesses of 'template_pool_' are in HdfsScanPlanNode::ProcessScanRangesAndInitSharedState(). This method is invoked in FragmentState::Init() (in PlanNode::CreateTree()) which is called before any fragment instances are created. So no races as well.
BTW, 'partition_template_tuple_map_' is also used in HdfsScanPlanNode::ProcessScanRangesAndInitSharedState() without the need of holding 'metadata_lock_'.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Dec 2022 00:25:12 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19289 )

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

Please resolve my comment in be/src/exec/avro/hdfs-avro-scanner.h

Otherwise LGTM!

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.h
File be/src/exec/avro/hdfs-avro-scanner.h:

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.h@124
PS1, Line 124: The ownership of memory is transferred to the scan-node pool,
> Please update this comment.
Please resolve this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 10:54:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/11924/ : 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/19289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Nov 2022 12:05:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/11928/ : 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/19289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Dec 2022 00:42:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19289 )

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 1:

(4 comments)

Left some minor comments, otherwise looks good to me!

http://gerrit.cloudera.org:8080/#/c/19289/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19289/1//COMMIT_MSG@47
PS1, Line 47: with
without?


http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.h
File be/src/exec/avro/hdfs-avro-scanner.h:

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.h@124
PS1, Line 124: The ownership of memory is transferred to the scan-node pool,
Please update this comment.


http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.cc
File be/src/exec/avro/hdfs-avro-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.cc@292
PS1, Line 292: no partition columns
nit: and no default values?


http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/hdfs-scan-node-base.cc@1305
PS1, Line 1305: metadata_lock_
Currently 'metadata_lock_' is used to protect 'per_file_metadata_'.

And 'template_pool_' has unprotected accesses in hdfs-scan-node-base.cc, e.g. in HdfsScanPlanNode::Close(). Are we sure it won't cause any concurrency issue?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Nov 2022 15:13:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 12:39:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 17:54:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Dec 2022 06:19:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Dec 2022 01:14:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/11966/ : 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/19289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 11:32:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Borok-Nagy, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................

IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Sequence container based file formats (SequenceFile, RCFile, Avro) have
a file header in each file that describes the metadata of the file, e.g.
codec, default values, etc. The header should be decoded before reading
the file content. The initial scanners will read the header and then
issue follow-up scan ranges for the file content. The decoded header
will be referenced by follow-up scanners.

Since IMPALA-9655, when MT_DOP > 1, the issued scan ranges could be
scheduled to other scan node instances. So the header resource should
live until all scan node instances close. Header objects are owned by
the object pool of the RuntimeState, which meets the requirement.

AvroFileHeader is special than other headers in that it references a
template tuple which contains the partition values and default values
for missing fields. The template tuple is initially owned by the header
scanner, then transferred to the scan node before the scanner closes.
However, when the scan node instance closes, the template tuple is
freed. Scanners of other scan node instances might still depend on it.
This could cause wrong results or crash the impalad.

When partition columns are used in the query, or when the underlying
avro files have missing fields and the table schema has default values
for them, the AvroFileHeader will have a non-null template tuple, which
could hit this bug when MT_DOP>1.

This patch fixes the bug by transferring the template tuple to
ScanRangeSharedState directly. The scan_node_pool of HdfsScanNodeBase is
also removed since it's only used to hold the template tuple (and
related buffers) of the avro header. Also no need to override
TransferToScanNodePool in HdfsScanNode since the original purpose is to
protect the pool by a lock, and now the method in ScanRangeSharedState
already has a lock.

Tests
 - Add missing test coverage for compute stats on avro tables. Note that
   MT_DOP=4 is set by default for compute stats.
 - Add the MT_DOP dimension for TestScannersAllTableFormats. Also add
   some queries that can reveal the bug in scanners.test. The ASAN build
   can easily crash by heap-use-after-free error without this fix.
 - Ran exhaustive tests.

Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
---
M be/src/exec/avro/hdfs-avro-scanner.cc
M be/src/exec/avro/hdfs-avro-scanner.h
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/streaming-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/compute-stats-avro.test
M testdata/workloads/functional-query/queries/QueryTest/scanners.test
M tests/query_test/test_scanners.py
12 files changed, 231 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/19289/3
-- 
To view, visit http://gerrit.cloudera.org:8080/19289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.h
File be/src/exec/avro/hdfs-avro-scanner.h:

http://gerrit.cloudera.org:8080/#/c/19289/1/be/src/exec/avro/hdfs-avro-scanner.h@124
PS1, Line 124: The ownership of memory is transferred to the template pool o
> Please resolve this.
Oops, sorry to miss this. Done.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 11:12:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Borok-Nagy, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState
......................................................................

IMPALA-11751: Template tuple of Avro header should be transferred to ScanRangeSharedState

Sequence container based file formats (SequenceFile, RCFile, Avro) have
a file header in each file that describes the metadata of the file, e.g.
codec, default values, etc. The header should be decoded before reading
the file content. The initial scanners will read the header and then
issue follow-up scan ranges for the file content. The decoded header
will be referenced by follow-up scanners.

Since IMPALA-9655, when MT_DOP > 1, the issued scan ranges could be
scheduled to other scan node instances. So the header resource should
live until all scan node instances close. Header objects are owned by
the object pool of the RuntimeState, which meets the requirement.

AvroFileHeader is special than other headers in that it references a
template tuple which contains the partition values and default values
for missing fields. The template tuple is initially owned by the header
scanner, then transferred to the scan node before the scanner closes.
However, when the scan node instance closes, the template tuple is
freed. Scanners of other scan node instances might still depend on it.
This could cause wrong results or crash the impalad.

When partition columns are used in the query, or when the underlying
avro files have missing fields and the table schema has default values
for them, the AvroFileHeader will have a non-null template tuple, which
could hit this bug when MT_DOP>1.

This patch fixes the bug by transferring the template tuple to
ScanRangeSharedState directly. The scan_node_pool of HdfsScanNodeBase is
also removed since it's only used to hold the template tuple (and
related buffers) of the avro header. Also no need to override
TransferToScanNodePool in HdfsScanNode since the original purpose is to
protect the pool by a lock, and now the method in ScanRangeSharedState
already has a lock.

Tests
 - Add missing test coverage for compute stats on avro tables. Note that
   MT_DOP=4 is set by default for compute stats.
 - Add the MT_DOP dimension for TestScannersAllTableFormats. Also add
   some queries that can reveal the bug in scanners.test. The ASAN build
   can easily crash by heap-use-after-free error without this fix.
 - Ran exhaustive tests.

Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
---
M be/src/exec/avro/hdfs-avro-scanner.cc
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/streaming-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/compute-stats-avro.test
M testdata/workloads/functional-query/queries/QueryTest/scanners.test
M tests/query_test/test_scanners.py
11 files changed, 228 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/19289/2
-- 
To view, visit http://gerrit.cloudera.org:8080/19289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iafa43fce7c2ffdc867004d11e5873327c3d8cb42
Gerrit-Change-Number: 19289
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>