You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2021/06/23 13:29:20 UTC

[Impala-ASF-CR] IMPALA-10765: Don't update catalog when inserting empty results on unpartitioned tables

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


Change subject: IMPALA-10765: Don't update catalog when inserting empty results on unpartitioned tables
......................................................................

IMPALA-10765: Don't update catalog when inserting empty results on unpartitioned tables

When no files are inserted to an unpartitioned table and the DML is not
specified with OVERWRITE, we don't need to update catalog since it takes
no effects. This avoid hitting IllegalStateException in
CatalogdOpExecutor#createInsertEvents().

Tests:
 - Manually verify the issue fixed
 - (TODO) Add e2e test
 - (TODO) run CORE test

Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
---
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/service/client-request-state.cc
3 files changed, 12 insertions(+), 4 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-10765: Allow firing insert events with empty files

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Hello Vihang Karajgaonkar, Zoltan Borok-Nagy, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-10765: Allow firing insert events with empty files
......................................................................

IMPALA-10765: Allow firing insert events with empty files

When INSERT or the INSERT part of a CTAS statement produces empty
results, catalogd will hit a Precondition check in createInsertEvents()
if the statement is not specified with OVERWRITE. This patch removes the
checks to relax the restriction. This is consistent with Hive since Hive
also fires insert events on empty insert results.

Tests
 - Add e2e test

Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M tests/custom_cluster/test_event_processing.py
2 files changed, 35 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@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>

[Impala-ASF-CR] IMPALA-10765: Allow firing insert events with empty files

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17629 )

Change subject: IMPALA-10765: Allow firing insert events with empty files
......................................................................


Patch Set 1:

(1 comment)

> Patch Set 1:
> 
> > I prefer to simply remove that Precondition. This came up earlier
>  > with Iceberg tables, at that time I preferred to keep it (don't
>  > remember why).
> 
> Yes, I hit this precondition check while testing my other patch https://gerrit.cloudera.org/#/c/17308/. I think we can remove the preconditions check.

I guess the Precondition was added to avoid firing meaningless insert events. 
Added e2e tests and found that both solutions work. However, Hive does fire insert events for empty results. So I think it's ok to be consistent. I adjust the solution to just remove the checks. We can do optimizations in later patches.

Thank Csaba and Vihang's quick review!

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

http://gerrit.cloudera.org:8080/#/c/17629/1//COMMIT_MSG@10
PS1, Line 10: we don't need to update catalog since it takes
            : no effect
> I am unsure whether this will work with transactional tables: in that case 
I think the coordinator will always abort the transaction at the end: https://github.com/apache/impala/blob/d99caa1f3a049fc5e20855f8e8bf846fd81f65c5/be/src/service/client-request-state.cc#L825

BTW, when no partitions are modified, we also skip calling updateCatalog().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Jun 2021 03:17:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10765: Allow firing insert events with empty files

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.

Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17629 )

Change subject: IMPALA-10765: Allow firing insert events with empty files
......................................................................


Patch Set 2:

(1 comment)

Thanks for working on this.

With this issue fixed maybe you could also resolve IMPALA-10744 by removing the condition 'catalog_.isEventProcessingActive()' in CatalogOpExecutor.shouldGenerateInsertEvents().

http://gerrit.cloudera.org:8080/#/c/17629/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17629/2//COMMIT_MSG@7
PS2, Line 7: with empty files
nit: "with no files written" or "with empty results"?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Jun 2021 09:48:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10765: Allow firing INSERT events with empty results

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

Change subject: IMPALA-10765: Allow firing INSERT events with empty results
......................................................................

IMPALA-10765: Allow firing INSERT events with empty results

When INSERT or the INSERT part of a CTAS statement produces empty
results, catalogd will hit a Precondition check in createInsertEvents()
if the statement is not specified with OVERWRITE. This patch removes the
checks to relax the restriction. This is consistent with Hive since Hive
also fires insert events on empty insert results.

Previously when event processor is disabled, we won't generate INSERT
events. This is inconsistent with other DDL events which will always be
generated by Hive. This patch removes the check on event processor's
status. So firing INSERT events is independent to the event processor.
This fixes IMPALA-10744.

Tests
 - Add e2e test

Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Reviewed-on: http://gerrit.cloudera.org:8080/17629
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 tests/custom_cluster/test_event_processing.py
2 files changed, 36 insertions(+), 6 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-10765: Allow firing insert events with empty files

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17629 )

Change subject: IMPALA-10765: Allow firing insert events with empty files
......................................................................


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Jun 2021 04:02:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10765: Allow firing insert events with empty files

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17629 )

Change subject: IMPALA-10765: Allow firing insert events with empty files
......................................................................


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Jun 2021 10:04:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10765: Allow firing insert events with empty files

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.

Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17629 )

Change subject: IMPALA-10765: Allow firing insert events with empty files
......................................................................


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Jun 2021 09:48:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10765: Allow firing INSERT events with empty results

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.

Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17629 )

Change subject: IMPALA-10765: Allow firing INSERT events with empty results
......................................................................


Patch Set 3: Code-Review+2

Thanks for fixing this.

Yeah, we found it during https://gerrit.cloudera.org/#/c/17560/ I even had a fix for it in one of the patch sets, but then we decided to file a separate fix for the bug, then it got forgotten... :( Sorry about that.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id708f38302b37d7e09435e7b8b0c9fc071659606
Gerrit-Change-Number: 17629
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Jun 2021 10:48:52 +0000
Gerrit-HasComments: No