Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/17842 )
Change subject: IMPALA-5741: Initial support for reading tiny RDBMS tables ...................................................................... Patch Set 16: (7 comments) http://gerrit.cloudera.org:8080/#/c/17842/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17842/16//COMMIT_MSG@13 PS16, Line 13: - It is not distributed. Does this mean the execution happens just on coordinator? Also, do you know why do we have this limitation? It sounds more like missing support rather than some technical/design limitation. http://gerrit.cloudera.org:8080/#/c/17842/16//COMMIT_MSG@23 PS16, Line 23: classpath and the minicluster cluster has been started. nit: "minicluster cluster" => minicluster or "minicluster cluster" => Impala cluster ? http://gerrit.cloudera.org:8080/#/c/17842/16/fe/src/test/java/org/apache/impala/service/FrontendTest.java File fe/src/test/java/org/apache/impala/service/FrontendTest.java: http://gerrit.cloudera.org:8080/#/c/17842/16/fe/src/test/java/org/apache/impala/service/FrontendTest.java@146 PS16, Line 146: assertEquals(4, resp.rows.size()); It's not obvious why the resp.rows.size() changed? http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/JdbcDataSource.java File java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/JdbcDataSource.java: http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/JdbcDataSource.java@96 PS16, Line 96: CREATED, Comments here briefly describing the different states would be good. Also, the possible state transitions. Can we go from CREATED->CLOSED ? http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/conf/JdbcStorageConfig.java File java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/conf/JdbcStorageConfig.java: http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/conf/JdbcStorageConfig.java@22 PS16, Line 22: DATABASE_TYPE("database.type", true), Comments for each property would be good. For instance what does the QUERY mean in this context? Or what's a COLUMN_MAPPING. Would be good to explain each property. http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/JdbcRecordIterator.java File java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/JdbcRecordIterator.java: http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/JdbcRecordIterator.java@96 PS16, Line 96: if (type.types.get(0).getType() != TTypeNodeType.SCALAR) { This seems like another limitation which we should mention - "No support for external tables with complex types". If an external table has both scalar and complex types and we want to only query the scalar columns would that use case still be supported? http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/util/QueryConditionUtil.java File java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/util/QueryConditionUtil.java: http://gerrit.cloudera.org:8080/#/c/17842/16/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/util/QueryConditionUtil.java@84 PS16, Line 84: } else { We're missing a bunch of types here Date, TimeStamp, Binary, Decimal,... Not sure what the TODO means? https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java#L139 ``` // TODO: we support DECIMAL, TIMESTAMP and DATE but no way to specify it in SQL. ``` -- To view, visit http://gerrit.cloudera.org:8080/17842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8244e978c7717c6f1452f66f1630b6441392e7d2 Gerrit-Change-Number: 17842 Gerrit-PatchSet: 16 Gerrit-Owner: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <gsi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 21 Sep 2023 20:16:11 +0000 Gerrit-HasComments: Yes
