Wenzhe Zhou 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 18: (8 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 Yes, it's single scan node and does not support partition. I could not find original design document. But from the comments in Jiras and discussion threads, it's technical/design limitation. http://gerrit.cloudera.org:8080/#/c/17842/16//COMMIT_MSG@22 PS16, Line 22: : In order Changed the code to load driver class from driver jar file so that driver jar files are not required to be in the classpath. Driver jar file has to be specified as the table property when creating data source table. http://gerrit.cloudera.org:8080/#/c/17842/16//COMMIT_MSG@23 PS16, Line 23: In order to query the RDBMS tables, the following steps should be > nit: fixed. 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? added alltypes_jdbc_datasource. 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: // are called in right order, e.g. state transitions must be in the below order: > Comments here briefly describing the different states would be good. Also, updated comments 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: // Table properties specified in the create table statement. > Comments for each property would be good. For instance what does the QUERY Added comments for 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 fo Add the limitation is commit messages. We should allow user to query the scalar columns. Will address it in following patches. 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,... I guess they were nor sure if the jdbc could could support Date, TimeStamp, Binary, Decimal data types for predicates at that time. Added TODO comment and declare the restrictions in commit messages. -- 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: 18 Gerrit-Owner: Fucun Chu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fucun Chu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Mon, 25 Sep 2023 00:31:59 +0000 Gerrit-HasComments: Yes
