Csaba Ringhofer 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 15: (4 comments) 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: if (tracker_ != nullptr) { : tracker_->Release(size_); : } We should also consider whether the allocation actually happened and was successful. My suggestion is to set size_ to 0 in the constructor and increase it in TryConsume based on a parameter if it was successful. This would allow calling TryConsume multiple times. 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: if (tbl instanceof FeIcebergTable) { I am unsure about the detailed verification. - I am not sure if FeIcebergTable should support any serde property in general - this strictness also disallows unsetting the property if it incorrectly set in another engine - the same verification should be done for create table too 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.com/apache/impala/blob/b121a40d20107bad6c04732ba580f26639acd43a/docker/quickstart_client/load_tpcds_parquet.sql#L78 for example. Meanwhile I am not sure about the serde prop verification, see my comment in AlterTableSetTblProperties.java 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: lambda v: v.get_value('table_format').file_format == 'text' > IIRC we have previously agreed to move SequenceFiles decoding to a separate It is not clear to me how this patch handles Sequence files - AlterTableSetTblProperties.java line 319 suggests that it is supported. -- 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: 15 Gerrit-Owner: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Mon, 31 Mar 2025 12:19:40 +0000 Gerrit-HasComments: Yes
