Pranav Lodha has posted comments on this change. ( http://gerrit.cloudera.org:8080/21805 )
Change subject: [WIP] IMPALA-12789: Fix unit-test code JdbcDataSourceTest.java ...................................................................... Patch Set 4: (15 comments) > Uploaded patch set 4. http://gerrit.cloudera.org:8080/#/c/21805/1/fe/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java File fe/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java: http://gerrit.cloudera.org:8080/#/c/21805/1/fe/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java@346 PS1, Line 346: ected int getFetchSize(Configuration conf) { : return conf : .getInt(JdbcStorageConfig.JDBC_FETCH_SIZE.getPropertyName(), DEFAULT_FETCH_SIZE); : } > dbcp_max_conn_pool_size, dbcp_max_wait_millis_for_conn and dbcp_data_source Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java File fe/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java: http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java@336 PS3, Line 336: props.put("maxTotal", > These indentation changes don't make sense. Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java File fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java: http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@37 PS3, Line 37: import org.apache.impala.thrift.TColumnData; > keep alphabet order Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@58 PS3, Line 58: pr > 2 indent spaces Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@62 PS3, Line 62: // > two indent spaces Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@78 PS3, Line 78: + "\ > 4 indent spaces Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@127 PS3, Line 127: params.setPredicates(predicates_); > this line is not needed since it will be overwrote by next line. Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@203 PS3, Line 203: id"); > rename local variable as 'schema' to distinguish with class member variable Done http://gerrit.cloudera.org:8080/#/c/21805/3/fe/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@266 PS3, Line 266: > this function is unused Done http://gerrit.cloudera.org:8080/#/c/21805/3/java/ext-data-source/jdbc/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java File java/ext-data-source/jdbc/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java: http://gerrit.cloudera.org:8080/#/c/21805/3/java/ext-data-source/jdbc/src/test/java/org/apache/impala/extdatasource/jdbc/JdbcDataSourceTest.java@91 PS3, Line 91: > This indentation change doesn't make our style. Done http://gerrit.cloudera.org:8080/#/c/21805/1/java/jamm-prefix/src/jamm File java/jamm-prefix/src/jamm: http://gerrit.cloudera.org:8080/#/c/21805/1/java/jamm-prefix/src/jamm@1 PS1, Line 1: > Agreed, this shouldn't be present. Removed http://gerrit.cloudera.org:8080/#/c/21805/3/java/jamm-prefix/src/jamm File java/jamm-prefix/src/jamm: http://gerrit.cloudera.org:8080/#/c/21805/3/java/jamm-prefix/src/jamm@1 PS3, Line 1: > This shouldn't exist, seems like you brought it in from an old branch. Done http://gerrit.cloudera.org:8080/#/c/21805/1/testdata/bin/create-ext-data-source-table.sql File testdata/bin/create-ext-data-source-table.sql: http://gerrit.cloudera.org:8080/#/c/21805/1/testdata/bin/create-ext-data-source-table.sql@31 PS1, Line 31: id INT, > unnecessary change Done http://gerrit.cloudera.org:8080/#/c/21805/3/testdata/bin/create-ext-data-source-table.sql File testdata/bin/create-ext-data-source-table.sql: http://gerrit.cloudera.org:8080/#/c/21805/3/testdata/bin/create-ext-data-source-table.sql@31 PS3, Line 31: id INT, > Why the indentation change? Doesn't match any other style here. Done http://gerrit.cloudera.org:8080/#/c/21805/3/testdata/bin/load-ext-data-sources.sh File testdata/bin/load-ext-data-sources.sh: http://gerrit.cloudera.org:8080/#/c/21805/3/testdata/bin/load-ext-data-sources.sh@101 PS3, Line 101: INSERT INTO test_strategy (strategy_id, name, referrer, landing, priority, implementation, last_modified) : VALUES : (1, 'S1', 'aaa', 'abc', 1000, NULL, '2012-05-08 15:01:15'), : (2, 'S2', 'bbb', 'def', 990, NULL, '2012-05-08 15:01:15'), : (3, 'S3', 'ccc', 'ghi', 1000, NULL, '2012-05-08 15:01:15'), : (4, 'S4', 'ddd', 'jkl', 980, NULL, '2012-05-08 15:01:15'), : (5, 'S5', 'eee', NULL, NULL, NULL, '2012-05-08 15:01:15'); : __EOT__ : sudo -u postgres psql -U hiveuser -d functional -f /tmp/jdbc_test_strategy.sql : : # Load data to jdbc table : cat ${IMPALA_HOME}/testdata/target/AllTypes/* > /tmp/jdbc_alltypes.csv : loadCmd="COPY alltypes FROM '/tmp/jdbc_alltypes.csv' DELIMITER ',' CSV" : sudo -u postgres psql -d functional -c "$loadCmd" : > One INSERT statement can insert multiple rows. Done -- To view, visit http://gerrit.cloudera.org:8080/21805 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie07173d256d73c88f5a6c041f087db16b6ff3127 Gerrit-Change-Number: 21805 Gerrit-PatchSet: 4 Gerrit-Owner: Pranav Lodha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Sat, 30 Nov 2024 16:42:32 +0000 Gerrit-HasComments: Yes
