impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aman Sinha (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-9602: Fix case-sensitivity for local catalog
Date Wed, 08 Apr 2020 01:31:25 GMT
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/15653 )

Change subject: IMPALA-9602: Fix case-sensitivity for local catalog
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15653/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15653/3//COMMIT_MSG@21
PS3, Line 21:    start Impala with local catalog enabled
            :     start-impala-cluster.py
            :      --catalogd_args="--catalog_topic_mode=minimal"
            :      --impalad_args="--use_local_catalog=true"
            :    Create database in lower-case: "CREATE DATABASE db1;"
            :    Run the following a few times (this errors without the patch):
            :    impala-shell.sh -q "DROP TABLE IF EXISTS DB1.ddl_test1 PURGE;
            :                   CREATE TABLE DB1.ddl_test1 (val string)
> We have a test_local_catalog.py or test_ddl.py in case you want to add this
That's good to know about the python script which seems great for end-to-end test and testing
race conditions in cache updates.  I will keep this in mind for future testing.  In this case
though, since the problem is simpler, I am thinking the unit test is good enough.  Let me
know if you think otherwise.


http://gerrit.cloudera.org:8080/#/c/15653/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java:

http://gerrit.cloudera.org:8080/#/c/15653/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@625
PS3, Line 625: DbCacheKey
> May be its better to move toLowerCase() call inside DbCacheKey constructor.
I had thought of that but looking at existing code in other places (e.g Catalog.java, CatalogObjectCache.java),
the caller explicitly lowercases instead of the constructor doing it.   One motivation for
it is there's a caseInsensitiveKeys_ flag in CatalogObjectCache to do this conditionally.
 I am thinking it would be good to maintain similar logic in the local catalog in case in
the future we want to do this conditionally for any reason.  What do you think ?



-- 
To view, visit http://gerrit.cloudera.org:8080/15653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f368fa9b50e22ec5057d0bf66c3fd51064d4c26
Gerrit-Change-Number: 15653
Gerrit-PatchSet: 3
Gerrit-Owner: Aman Sinha <amsinha@cloudera.com>
Gerrit-Reviewer: Aman Sinha <amsinha@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vihang@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 01:31:25 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message