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 2021/03/31 15:39:45 UTC

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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


Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................

IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format

During table creation we pushed all the table properties to the
Iceberg table, e.g. 'iceberg.file_format'. This works well with
current Iceberg version, but causes bugs with newer versions.

Newer versions of Iceberg refresh the HMS properties from the
table properties for every table update. That means if we
intially set a table property, we cannot modify it later because
Iceberg will rewrite it with the old value on every table update.

These table properties are not needed at the Iceberg table level,
so we can just skip passing them during table creation.

Testing:
 * ran existing tests
 * ran tests with newer version Iceberg
 * checked manually that we don't set these properties at the Iceberg
   table level

Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
2 files changed, 31 insertions(+), 4 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17248/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/17248/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@177
PS1, Line 177: uld only 
> Thanks, I've added your version.
Thanks



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 01 Apr 2021 17:45:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17248/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/17248/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@177
PS1, Line 177: uld only 
> Not sure if I understand this comment fully. Is this more readable?
Thanks, I've added your version.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 01 Apr 2021 08:58:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................

IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format

During table creation we pushed all the table properties to the
Iceberg table, e.g. 'iceberg.file_format'. This works well with
current Iceberg version, but causes bugs with newer versions.

Newer versions of Iceberg refresh the HMS properties from the
table properties for every table update. That means if we
intially set a table property, we cannot modify it later because
Iceberg will rewrite it with the old value on every table update.

These table properties are not needed at the Iceberg table level,
so we can just skip passing them during table creation.

Testing:
 * ran existing tests
 * ran tests with newer version Iceberg
 * checked manually that we don't set these properties at the Iceberg
   table level

Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Reviewed-on: http://gerrit.cloudera.org:8080/17248
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
2 files changed, 32 insertions(+), 4 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 01 Apr 2021 17:46:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 31 Mar 2021 16:00:07 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Gabor Kaszab, wangsheng, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................

IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format

During table creation we pushed all the table properties to the
Iceberg table, e.g. 'iceberg.file_format'. This works well with
current Iceberg version, but causes bugs with newer versions.

Newer versions of Iceberg refresh the HMS properties from the
table properties for every table update. That means if we
intially set a table property, we cannot modify it later because
Iceberg will rewrite it with the old value on every table update.

These table properties are not needed at the Iceberg table level,
so we can just skip passing them during table creation.

Testing:
 * ran existing tests
 * ran tests with newer version Iceberg
 * checked manually that we don't set these properties at the Iceberg
   table level

Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
2 files changed, 32 insertions(+), 4 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 01 Apr 2021 17:46:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 01 Apr 2021 23:37:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 1:

(1 comment)

Patch looks good to me. I left a comment regarding the comment.

http://gerrit.cloudera.org:8080/#/c/17248/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/17248/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@177
PS1, Line 177: only be s
Not sure if I understand this comment fully. Is this more readable?
Returns true if the table property should only be stored in HMS. If false, the property is stored in HMS as well as iceberg?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:38:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10624: TestIcebergTable::test alter iceberg tables failed by stale file format

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

Change subject: IMPALA-10624: TestIcebergTable::test_alter_iceberg_tables failed by stale file format
......................................................................


Patch Set 2:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff8d5d1d90444aba11d47cfce522aaa45a4a74cc
Gerrit-Change-Number: 17248
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Thu, 01 Apr 2021 09:15:56 +0000
Gerrit-HasComments: No