You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2022/07/05 14:36:36 UTC

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18704


Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M be/src/exec/scratch-tuple-batch.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
28 files changed, 619 insertions(+), 74 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 10:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 17:59:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 5:

Build Failed 

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 17:23:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lipenglin@sensorsdata.cn has posted comments on this change. ( http://gerrit.cloudera.org:8080/18704 )

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/18704/2/be/src/exec/hdfs-scan-node-base.cc@999
PS2, Line 999: else if (sd->virtual_column_type() == TVirtualColumnType::FILE_POSITION) {
             :       return false;
> No, in the template tuple of a scanner we only set slots that are constant 
Got it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Jul 2022 14:23:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 8:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Aug 2022 11:55:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 17:04:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/18704/1/be/src/exec/parquet/parquet-collection-column-reader.cc
File be/src/exec/parquet/parquet-collection-column-reader.cc:

http://gerrit.cloudera.org:8080/#/c/18704/1/be/src/exec/parquet/parquet-collection-column-reader.cc@84
PS1, Line 84:           pos_slot_desc() ? tuple->GetBigIntSlot(pos_slot_desc()->tuple_offset()) : nullptr,
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/1/be/src/exec/parquet/parquet-collection-column-reader.cc@85
PS1, Line 85:           file_pos_slot_desc() ? tuple->GetBigIntSlot(file_pos_slot_desc()->tuple_offset()) : nullptr);
line too long (103 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/1/be/src/exec/parquet/parquet-column-readers.cc
File be/src/exec/parquet/parquet-column-readers.cc:

http://gerrit.cloudera.org:8080/#/c/18704/1/be/src/exec/parquet/parquet-column-readers.cc@1059
PS1, Line 1059:     const parquet::ColumnChunk& col_chunk, int row_group_idx, int64_t row_group_first_row) {
line too long (92 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Jul 2022 14:37:33 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Reviewed-on: http://gerrit.cloudera.org:8080/18704
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/base-sequence-scanner.h
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M testdata/LineItemMultiBlock/README.dox
A testdata/LineItemMultiBlock/lineitem_multiblock_variable_num_rows.parquet
M testdata/bin/create-load-data.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
35 files changed, 856 insertions(+), 83 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, lipenglin@sensorsdata.cn, Gergely Fürnstáhl, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.h
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
A testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
30 files changed, 698 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/18704/8
-- 
To view, visit http://gerrit.cloudera.org:8080/18704
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18704/4/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/4/tests/query_test/test_scanners.py@156
PS4, Line 156: test_virtual_column_file_position_generic
Could you add some tests that queries both file__position and input__file__name at the same time?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jul 2022 12:42:33 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 10:

(9 comments)

Thanks for the comments!

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

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/hdfs-scan-node-base.cc@1042
PS8, Line 1042:     } else if (sd->virtual_column_type() == TVirtualColumnType::FILE_POSITION) {
> nit: you could do a 'continue here' instead of no-op.
Done


http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.h
File be/src/exec/parquet/hdfs-parquet-scanner.h:

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.h@820
PS8, Line 820:   /// row in this row group, it is used to track the file position of the rows.
> Could you comment the purpose of 'row_group_first_row' parameter?
Done


http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.cc
File be/src/exec/parquet/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@856
PS8, Line 856: i];
> Bit confused but shouldn't this index should be 'i' instead of 'group_idx_'
Ah, good catch, thanks, you saved a couple of debugging hours for us.. :)

Unfortunately the row groups of functional_parquet.lineitem_multiblocks have the same number of rows (100). I've added a new Parquet file lineitem_multiblocks_variable_num_rows and some tests.


http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/parquet-column-readers.h
File be/src/exec/parquet/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/parquet-column-readers.h@174
PS8, Line 174: Only valid to call when doing non-batched reading
> This part might be misleading as this is also called in CollectionColumnRea
Yeah it's a bit confusing, but CollectionColumnReader::ReadValueBatch() invokes the non-batched interfaces of the column-readers (e.g. NextLevels(), ReadValue(), ReadSlots()). Plus already had the same comment for 'ReadPositionNonBatched' in Base.


http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:

http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogObjects.thrift@77
PS8, Line 77: enum TVirtualColumnType {
> Since TVirtualColumnType is optional in TPartialTableInfo I wonder if there
In the FE we use the NONE value at some places. It would be possible to use NULL values instead I think, but I'm not sure if it worth doing this in the context of this change.


http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:

http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogService.thrift@471
PS8, Line 471:   4: optional list<CatalogObjects.TColumn> virtual_columns
> Was there any reason this new member came to the 4th position and not at th
It just seemed more logical to list it earlier than some other fields.


http://gerrit.cloudera.org:8080/#/c/18704/8/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java:

http://gerrit.cloudera.org:8080/#/c/18704/8/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@417
PS8, Line 417:   private void addVirtualColumns() {
> This function seems the same as the one in HdfsTable. Can this be moved to 
Non-FS tables also extend the Table class.

I don't really want to further complicate the class hierarchy with a new base class just because of this small method. Especially if we add different virtual columns to the different FS-tables. E.g. if we add SEQUENCE_NUMBER or some other Iceberg-specific column.


http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
File testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test:

http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test@101
PS8, Line 101: ====
> Could you please add a test where file__position is present multiple times 
Done


http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
File testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test:

http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test@116
PS8, Line 116: ---- RESULTS
> Would it be a valid use case when two tables are joined and we get the file
Sure, added few queries.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 17:39:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18704/6/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/6/be/src/exec/hdfs-scanner.cc@80
PS6, Line 80: // Virtual column FILE__POSITION is only supported for PARQUET files.
> Sounds like an okayish ramp-up task to implement it for other file formats.
We will need it for ORC in the near future, so might not want to wait with that.

For other splittable file formats it might not be possible to implement this without turning of file splitting.


http://gerrit.cloudera.org:8080/#/c/18704/6/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/6/tests/query_test/test_scanners.py@164
PS6, Line 164: test_mixing_virtual_columns
> Thanks for adding this, additionally I observed that the input__file__name 
Thanks for catching this. Actually the bug was triggered by the order of the virtual columns, i.e. a simpler query was also broken:

 select file__position, input__file__name, id
 from functional_parquet.alltypestiny;

The bug was in HdfsScanPlanNode::HasVirtualColumnInTemplateTuple(). Added some tests as well.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Aug 2022 09:16:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 15:19:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, lipenglin@sensorsdata.cn, Gergely Fürnstáhl, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.h
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
A testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
30 files changed, 661 insertions(+), 77 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/18704/5
-- 
To view, visit http://gerrit.cloudera.org:8080/18704
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lipenglin@sensorsdata.cn has posted comments on this change. ( http://gerrit.cloudera.org:8080/18704 )

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/18704/2/be/src/exec/hdfs-scan-node-base.cc@999
PS2, Line 999: else if (sd->virtual_column_type() == TVirtualColumnType::FILE_POSITION) {
             :       return false;
Should this return true?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Wed, 06 Jul 2022 11:28:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 7: Code-Review+1

Thanks for the update, LGTM!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Aug 2022 15:46:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 8: Code-Review+1

PS8 is only a rebase.

Carrying +1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Aug 2022 11:34:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, lipenglin@sensorsdata.cn, Gergely Fürnstáhl, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.h
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
A testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
30 files changed, 699 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/18704/7
-- 
To view, visit http://gerrit.cloudera.org:8080/18704
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 4:

(7 comments)

I quickly went through the code except the tests. Will make another round next week.

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/base-sequence-scanner.cc
File be/src/exec/base-sequence-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/base-sequence-scanner.cc@101
PS4, Line 101:   for (SlotDescriptor* sd : scan_node_->virtual_column_slots()) {
I see very similar code in multiple scanner types. Can you move this to a common base class?


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/hdfs-text-scanner.cc@820
PS4, Line 820:   for (SlotDescriptor* sd : scan_node_->virtual_column_slots()) {
Just curious: can't we decide in analysis time that we're going to have some files in scanning that don't support FILE__POSITION?


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h
File be/src/exec/parquet/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h@415
PS4, Line 415: When updated, and its value is N, it means that we already processed the Nth row
             :   /// completely.
Do I get it right that this also means that current_row_ starts from zero where -1 is for invalid value and zero is for the first row? Could you please also add this to the comment?


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h@667
PS4, Line 667: int64_t* pos, int64_t* file_pos
This approach with 2 mutually exclusive params seems weird for me. Shouldn't this be 2 separate functions, one for pos_slot and one for file_pos_slot?


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc
File be/src/exec/parquet/parquet-column-readers.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc@576
PS4, Line 576: ReadPositionBatched<true>(rep_level, nullptr,
             :           tuple->GetBigIntSlot(file_pos_slot_desc_->tuple_offset()));
Similary, if we always set one of the pos params null at the callsite and the logic is kind of different for each pos column inside the functions, shouldn't this be 2 separate functions?


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc@931
PS4, Line 931: true
At the callsite it's not that easy to figure out what this new bool parameter is for. If it's feasible I'd vote for split up the function for the 2 pos cols.


http://gerrit.cloudera.org:8080/#/c/18704/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:

http://gerrit.cloudera.org:8080/#/c/18704/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@186
PS4, Line 186:       addVirtualColumn(virtCol);
nit: this should fit into the line above.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Jul 2022 14:13:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/18704/3/be/src/exec/parquet/parquet-collection-column-reader.cc
File be/src/exec/parquet/parquet-collection-column-reader.cc:

http://gerrit.cloudera.org:8080/#/c/18704/3/be/src/exec/parquet/parquet-collection-column-reader.cc@84
PS3, Line 84:           pos_slot_desc() ? tuple->GetBigIntSlot(pos_slot_desc()->tuple_offset()) : nullptr,
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/3/be/src/exec/parquet/parquet-collection-column-reader.cc@85
PS3, Line 85:           file_pos_slot_desc() ? tuple->GetBigIntSlot(file_pos_slot_desc()->tuple_offset()) : nullptr);
line too long (103 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/3/be/src/exec/parquet/parquet-column-readers.cc
File be/src/exec/parquet/parquet-column-readers.cc:

http://gerrit.cloudera.org:8080/#/c/18704/3/be/src/exec/parquet/parquet-column-readers.cc@1059
PS3, Line 1059:     const parquet::ColumnChunk& col_chunk, int row_group_idx, int64_t row_group_first_row) {
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/3/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/3/tests/query_test/test_scanners.py@142
PS3, Line 142: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/18704/3/tests/query_test/test_scanners.py@428
PS3, Line 428: l
flake8: E126 continuation line over-indented for hanging indent



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jul 2022 15:50:20 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 3: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8301/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jul 2022 16:56:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/18704/10/testdata/bin/create-load-data.sh
File testdata/bin/create-load-data.sh:

http://gerrit.cloudera.org:8080/#/c/18704/10/testdata/bin/create-load-data.sh@536
PS10, Line 536:     ${IMPALA_HOME}/testdata/LineItemMultiBlock/lineitem_multiblock_variable_num_rows.parquet \
line too long (94 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/10/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/10/tests/query_test/test_scanners.py@143
PS10, Line 143: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/18704/10/tests/query_test/test_scanners.py@438
PS10, Line 438: l
flake8: E126 continuation line over-indented for hanging indent



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 17:39:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 5:

(13 comments)

Thanks for the comments!

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/base-sequence-scanner.cc
File be/src/exec/base-sequence-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/base-sequence-scanner.cc@101
PS4, Line 101:   bytes_skipped_counter_ = ADD_COUNTER(
> I see very similar code in multiple scanner types. Can you move this to a c
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/hdfs-text-scanner.cc@820
PS4, Line 820:   RETURN_IF_ERROR(InitNewRange());
> Just curious: can't we decide in analysis time that we're going to have som
Currently we add virtual columns to the table's schema during schema loading, which happens before partition loading when we get information about the underlying file formats.

Maybe we could add virtual columns later, but probably it's still useful to have this check here anyway.


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/hdfs-parquet-scanner.cc
File be/src/exec/parquet/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/hdfs-parquet-scanner.cc@852
PS4, Line 852:     // Let's find the index of the first row in this row group. It's needed to track the
> nit: In this method almost every operation has an explanatory comment. Coul
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-collection-column-reader.cc
File be/src/exec/parquet/parquet-collection-column-reader.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-collection-column-reader.cc@84
PS4, Line 84:     } else if (file_pos_slot_desc() != nullptr) {
> line too long (92 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-collection-column-reader.cc@85
PS4, Line 85:       ReadFilePositionNonBatched(
> line too long (103 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h
File be/src/exec/parquet/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h@221
PS4, Line 221: 
             :   /// The slot descriptors for the collection item position and file position fields of
             :   /// the tuple, if there is one. NULL if there's not.
> nit: If I understand it well, these are mutually exclusive fields and one o
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h@415
PS4, Line 415: t parquet::ColumnMetaData* metadata_ = nullptr;
             : 
> Do I get it right that this also means that current_row_ starts from zero w
Added some extra comment.


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h@667
PS4, Line 667: 
> This approach with 2 mutually exclusive params seems weird for me. Shouldn'
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc
File be/src/exec/parquet/parquet-column-readers.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc@576
PS4, Line 576: 
             : 
> Similary, if we always set one of the pos params null at the callsite and t
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc@931
PS4, Line 931: _rea
> At the callsite it's not that easy to figure out what this new bool paramet
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc@1059
PS4, Line 1059:       parent_->filename(), row_group_idx, col_idx(), schema_element(),
> line too long (92 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java:

http://gerrit.cloudera.org:8080/#/c/18704/4/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@186
PS4, Line 186:   }
> nit: this should fit into the line above.
Done


http://gerrit.cloudera.org:8080/#/c/18704/4/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/4/tests/query_test/test_scanners.py@156
PS4, Line 156: test_virtual_column_file_position_generic
> Could you add some tests that queries both file__position and input__file__
Added a new test that uses both.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 17:03:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 6: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8393/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Aug 2022 17:16:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Aug 2022 14:19:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, lipenglin@sensorsdata.cn, Gergely Fürnstáhl, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.h
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M testdata/LineItemMultiBlock/README.dox
A testdata/LineItemMultiBlock/lineitem_multiblock_variable_num_rows.parquet
M testdata/bin/create-load-data.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
35 files changed, 856 insertions(+), 83 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 14:56:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-collection-column-reader.cc
File be/src/exec/parquet/parquet-collection-column-reader.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-collection-column-reader.cc@84
PS4, Line 84:           pos_slot_desc() ? tuple->GetBigIntSlot(pos_slot_desc()->tuple_offset()) : nullptr,
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-collection-column-reader.cc@85
PS4, Line 85:           file_pos_slot_desc() ? tuple->GetBigIntSlot(file_pos_slot_desc()->tuple_offset()) : nullptr);
line too long (103 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc
File be/src/exec/parquet/parquet-column-readers.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.cc@1059
PS4, Line 1059:     const parquet::ColumnChunk& col_chunk, int row_group_idx, int64_t row_group_first_row) {
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/4/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/4/tests/query_test/test_scanners.py@142
PS4, Line 142: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/18704/4/tests/query_test/test_scanners.py@428
PS4, Line 428: l
flake8: E126 continuation line over-indented for hanging indent



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 16:07:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 16:06:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 6:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Aug 2022 12:22:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 6:

(2 comments)

Thanks for the changes Zoltan!

http://gerrit.cloudera.org:8080/#/c/18704/6/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/6/be/src/exec/hdfs-scanner.cc@80
PS6, Line 80: // Virtual column FILE__POSITION is only supported for PARQUET files.
Sounds like an okayish ramp-up task to implement it for other file formats.


http://gerrit.cloudera.org:8080/#/c/18704/6/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/6/tests/query_test/test_scanners.py@164
PS6, Line 164: test_mixing_virtual_columns
Thanks for adding this, additionally I observed that the input__file__name remains empty when I add a join or use order by.

For example:
 select a.file__position, a.input__file__name, a.id
 from functional_parquet.alltypestiny a, functional_parquet.alltypestiny b;
 +----------------+-------------------+----+
 | file__position | input__file__name | id |
 +----------------+-------------------+----+
 | 0              |                   | 2  |
 ...
 | 1              |                   | 1  |
 ...



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Aug 2022 08:23:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18704/5/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/5/tests/query_test/test_scanners.py@142
PS5, Line 142: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/18704/5/tests/query_test/test_scanners.py@437
PS5, Line 437: l
flake8: E126 continuation line over-indented for hanging indent



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 17:04:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jul 2022 15:49:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, lipenglin@sensorsdata.cn, Gergely Fürnstáhl, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M be/src/exec/scratch-tuple-batch.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
28 files changed, 591 insertions(+), 69 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 2: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8292/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 20:07:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/18704/2/be/src/exec/hdfs-scan-node-base.cc@999
PS2, Line 999: else if (sd->virtual_column_type() == TVirtualColumnType::FILE_POSITION) {
             :       return false;
> Should this return true?
No, in the template tuple of a scanner we only set slots that are constant for the file being scanned. E.g. partition slots, INPUT__FILE__NAME, etc.

FILE_POSITION is different for each row, so we cannot set it in the template tuple.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Jul 2022 11:38:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 4:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 16:26:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, lipenglin@sensorsdata.cn, Gergely Fürnstáhl, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
27 files changed, 590 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/18704/4
-- 
To view, visit http://gerrit.cloudera.org:8080/18704
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 4: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8304/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 20:49:55 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 10:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 17:39:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 11: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Aug 2022 14:26:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 7:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Aug 2022 09:35:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 5: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8388/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Aug 2022 12:07:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, lipenglin@sensorsdata.cn, Gergely Fürnstáhl, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................

IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables

Virtual column FILE__POSITION returns the ordinal position of the row
in the data file. It will be useful to add support for Iceberg's
position-based delete files

This patch only adds FILE__POSITION to Parquet tables. It works
similarly to the handling of collection position slots. I.e. we
add the responsibility of dealing with the file position slot to
an existing column reader. Because of page-filtering and late
materialization we already tracked the file position in member
'current_row_' during scanning.

Querying the FILE__POSITION in other file formats raises an error.

Testing:
 * added e2e tests

Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
---
M be/src/exec/base-sequence-scanner.h
M be/src/exec/file-metadata-utils.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/parquet-collection-column-reader.cc
M be/src/exec/parquet/parquet-column-readers.cc
M be/src/exec/parquet/parquet-column-readers.h
M common/thrift/CatalogObjects.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/VirtualColumn.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M testdata/LineItemMultiBlock/README.dox
A testdata/LineItemMultiBlock/lineitem_multiblock_variable_num_rows.parquet
M testdata/bin/create-load-data.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-generic.test
A testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
M tests/query_test/test_scanners.py
35 files changed, 861 insertions(+), 83 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 10: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 22:31:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 10: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Aug 2022 15:06:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 11:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Aug 2022 14:26:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 11: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Aug 2022 19:21:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/18704/9/testdata/bin/create-load-data.sh
File testdata/bin/create-load-data.sh:

http://gerrit.cloudera.org:8080/#/c/18704/9/testdata/bin/create-load-data.sh@536
PS9, Line 536:     ${IMPALA_HOME}/testdata/LineItemMultiBlock/lineitem_multiblock_variable_num_rows.parquet \
line too long (94 > 90)


http://gerrit.cloudera.org:8080/#/c/18704/9/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/9/tests/query_test/test_scanners.py@143
PS9, Line 143: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/18704/9/tests/query_test/test_scanners.py@438
PS9, Line 438: l
flake8: E126 continuation line over-indented for hanging indent



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 17:36:46 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 9:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 17:59:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 3:

Build Failed 

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jul 2022 16:00:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18704/7/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/7/tests/query_test/test_scanners.py@142
PS7, Line 142: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/18704/7/tests/query_test/test_scanners.py@437
PS7, Line 437: l
flake8: E126 continuation line over-indented for hanging indent



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Aug 2022 09:15:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 7:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Aug 2022 09:17:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 4:

(2 comments)

Hi Zoli, this change looks great. Added two nits where some additional comments could help understanding the code faster.
I would like to check it once more, because I have less experience with the column readers.

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/hdfs-parquet-scanner.cc
File be/src/exec/parquet/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/hdfs-parquet-scanner.cc@852
PS4, Line 852:     int64_t row_group_first_row = 0;
nit: In this method almost every operation has an explanatory comment. Could we add a line here as well?


http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h
File be/src/exec/parquet/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/18704/4/be/src/exec/parquet/parquet-column-readers.h@221
PS4, Line 221:   const SlotDescriptor* pos_slot_desc_ = nullptr;
             : 
             :   const SlotDescriptor* file_pos_slot_desc_ = nullptr;
nit: If I understand it well, these are mutually exclusive fields and one of them has to be non null.
In hdfs-parquet-scanner.cc:2735 the mutually exclusive part is explained for variables, could we have a some additional notes here as well?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Jul 2022 09:59:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18704/8/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/8/tests/query_test/test_scanners.py@143
PS8, Line 143: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/18704/8/tests/query_test/test_scanners.py@438
PS8, Line 438: l
flake8: E126 continuation line over-indented for hanging indent



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Aug 2022 11:34:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11350: Add virtual column FILE POSITION for Parquet tables

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

Change subject: IMPALA-11350: Add virtual column FILE__POSITION for Parquet tables
......................................................................


Patch Set 8:

(10 comments)

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

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/hdfs-scan-node-base.cc@1042
PS8, Line 1042:     } else if (sd->virtual_column_type() == TVirtualColumnType::FILE_POSITION) {
nit: you could do a 'continue here' instead of no-op.


http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.h
File be/src/exec/parquet/hdfs-parquet-scanner.h:

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.h@820
PS8, Line 820:   Status InitScalarColumns(int64_t row_group_first_row) WARN_UNUSED_RESULT;
Could you comment the purpose of 'row_group_first_row' parameter?


http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.cc
File be/src/exec/parquet/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@856
PS8, Line 856: group_idx_
Bit confused but shouldn't this index should be 'i' instead of 'group_idx_'?
This would only work if all the row group below the index 'group_idx_' has the same number of rows. If this is the case then I think the loop isn't needed.


http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/parquet-column-readers.h
File be/src/exec/parquet/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/18704/8/be/src/exec/parquet/parquet-column-readers.h@174
PS8, Line 174: Only valid to call when doing non-batched reading
This part might be misleading as this is also called in CollectionColumnReader::ReadValueBatch()


http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:

http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogObjects.thrift@77
PS8, Line 77: enum TVirtualColumnType {
Since TVirtualColumnType is optional in TPartialTableInfo I wonder if there is any use of the "NONE" value of this enum.


http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:

http://gerrit.cloudera.org:8080/#/c/18704/8/common/thrift/CatalogService.thrift@471
PS8, Line 471:   4: optional list<CatalogObjects.TColumn> virtual_columns
Was there any reason this new member came to the 4th position and not at the end?


http://gerrit.cloudera.org:8080/#/c/18704/8/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java
File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java:

http://gerrit.cloudera.org:8080/#/c/18704/8/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@417
PS8, Line 417:   private void addVirtualColumns() {
This function seems the same as the one in HdfsTable. Can this be moved to a common parent class, e.g. Table? (Or to a new common parent class, something like TableWithVirtualColumns?)


http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test
File testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test:

http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/mixing-virtual-columns.test@101
PS8, Line 101: ====
Could you please add a test where file__position is present multiple times in the select list?


http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test
File testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test:

http://gerrit.cloudera.org:8080/#/c/18704/8/testdata/workloads/functional-query/queries/QueryTest/virtual-column-file-position-parquet.test@116
PS8, Line 116: ====
Would it be a valid use case when two tables are joined and we get the file__position from both them (table1.file__position, table2.file__position)?


http://gerrit.cloudera.org:8080/#/c/18704/8/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/18704/8/tests/query_test/test_scanners.py@160
PS8, Line 160: TODO
nit: addig a Jira ID here would be nice.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ef72c683d0d5ae2898bca36fa87e74b663671f7
Gerrit-Change-Number: 18704
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 11:53:28 +0000
Gerrit-HasComments: Yes