Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22049 )
Change subject: WIP IMPALA-10319: Support arbitrary encodings on Text/Sequence files ...................................................................... Patch Set 16: (4 comments) - fixed issues in threads - improved test to use charset dimensions instead of loops http://gerrit.cloudera.org:8080/#/c/22049/15/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/22049/15/be/src/runtime/mem-tracker.cc@587 PS15, Line 587: : ScopedMemTracker::~ScopedMemTracker() { : i > We should also consider whether the allocation actually happened and was su Good point, fixed. http://gerrit.cloudera.org:8080/#/c/22049/14/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java: http://gerrit.cloudera.org:8080/#/c/22049/14/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@297 PS14, Line 297: > I am unsure about the detailed verification. Ok, I took this from a similiar verification but probably not applicable here indeed. Removing this and the check above. http://gerrit.cloudera.org:8080/#/c/22049/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/22049/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1068 PS14, Line 1068: "alter table > CREATE TABLE ... SERDEPROPERTIES could be also tested. See https://github.c You're right, I've overlooked 'CREATE TABLE WITH' syntax. Added necessary check to CreateTableStmt.java and tests. http://gerrit.cloudera.org:8080/#/c/22049/14/tests/query_test/test_charcodec.py File tests/query_test/test_charcodec.py: http://gerrit.cloudera.org:8080/#/c/22049/14/tests/query_test/test_charcodec.py@77 PS14, Line 77: ########################################################### > It is not clear to me how this patch handles Sequence files - AlterTableSet You're right, that was added prematurely. Removing for now. -- To view, visit http://gerrit.cloudera.org:8080/22049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I787cd01caa52a19d6645519a6cedabe0a5253a65 Gerrit-Change-Number: 22049 Gerrit-PatchSet: 16 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 15 Apr 2025 19:49:40 +0000 Gerrit-HasComments: Yes
