Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20915 )
Change subject: IMPALA-12503: Support date data type for predicates for external data source table ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/20915/6/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java File java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java: http://gerrit.cloudera.org:8080/#/c/20915/6/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java@66 PS6, Line 66: long define as 'static final long' type for this constant. Also move this line after line #73 http://gerrit.cloudera.org:8080/#/c/20915/6/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java@214 PS6, Line 214: mm nit: yyyy-MM-dd to keep consistent with code in the function http://gerrit.cloudera.org:8080/#/c/20915/6/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/dao/GenericJdbcDatabaseAccessor.java@220 PS6, Line 220: nit: two more indent space http://gerrit.cloudera.org:8080/#/c/20915/6/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/20915/6/java/ext-data-source/jdbc/src/main/java/org/apache/impala/extdatasource/jdbc/util/QueryConditionUtil.java@28 PS6, Line 28: import org.apache.impala.extdatasource.jdbc.dao.DatabaseAccessor; nit: keep alphabet order http://gerrit.cloudera.org:8080/#/c/20915/6/testdata/bin/clean-impala-env.sh File testdata/bin/clean-impala-env.sh: http://gerrit.cloudera.org:8080/#/c/20915/6/testdata/bin/clean-impala-env.sh@1 PS6, Line 1: #!/bin/bash Don't need this script to clean up if we combine setup-impala-env.sh with testdata/bin/load-ext-data-sources.sh http://gerrit.cloudera.org:8080/#/c/20915/6/testdata/bin/setup-impala-env.sh File testdata/bin/setup-impala-env.sh: http://gerrit.cloudera.org:8080/#/c/20915/6/testdata/bin/setup-impala-env.sh@29 PS6, Line 29: # Create local impala tables You can combine this script with testdata/bin/load-ext-data-sources.sh, which create table and load data for Postgres now so that we don't need a new script. http://gerrit.cloudera.org:8080/#/c/20915/6/testdata/bin/setup-impala-env.sh@32 PS6, Line 32: alltypes_impala name the table as alltypes_jdbc_impala_datasource http://gerrit.cloudera.org:8080/#/c/20915/6/testdata/bin/setup-impala-env.sh@49 PS6, Line 49: cat > /tmp/impala_jdbc_alltypes_with_case_sensitive_names.sql <<__EOT__ : USE FUNCTIONAL; : DROP TABLE IF EXISTS AllTypesCaseSensitiveNames_impala; : CREATE TABLE AllTypesCaseSensitiveNames_impala : ( : id INT, : Bool_col BOOLEAN, : Tinyint_col SMALLINT, : Smallint_col SMALLINT, : Int_col INT, : Bigint_col BIGINT, : Float_col FLOAT, : Double_col DOUBLE, : Date_col DATE, : String_col VARCHAR(10), : Timestamp_col TIMESTAMP : ); : __EOT_ Impala is case insensitive. All database names and table names are converted to lower case internally. So don't need to create this table. http://gerrit.cloudera.org:8080/#/c/20915/6/tests/custom_cluster/test_ext_data_sources.py File tests/custom_cluster/test_ext_data_sources.py: http://gerrit.cloudera.org:8080/#/c/20915/6/tests/custom_cluster/test_ext_data_sources.py@209 PS6, Line 209: # create alltypes_impala and load data into it. : script = os.path.join(os.environ['IMPALA_HOME'], 'testdata/bin/setup-impala-env.sh') : run_cmd = [script] : try: : subprocess.check_call(run_cmd, close_fds=True) : except subprocess.CalledProcessError: : assert False, "Failed to setup impala env" : : @classmethod : def _remove_impala_test_env(cls): : # drop table alltypes_impala : script = os.path.join(os.environ['IMPALA_HOME'], 'testdata/bin/clean-impala-env.sh') : run_cmd = [script] : subprocess.check_call(run_cmd, close_fds=True) : try: : subprocess.check_call(run_cmd, close_fds=True) : except subprocess.CalledProcessError: : assert False, "Failed to clean impala env" Don't need this functions if we combine setup-impala-env.sh with testdata/bin/load-ext-data-sources.sh -- To view, visit http://gerrit.cloudera.org:8080/20915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf13cbefaad812a0f78755c5791d82b24a3395e4 Gerrit-Change-Number: 20915 Gerrit-PatchSet: 6 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Wed, 24 Jan 2024 22:12:09 +0000 Gerrit-HasComments: Yes
