Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/9306 )
Change subject: IMPALA-5752: Add support for DECIMAL on Kudu tables ...................................................................... Patch Set 2: (5 comments) > (1 comment) > > I ran the code formatter and updated the patch with those changes. > > I think the runtime filter functionality should be added, but I > should likely do that in a separate patch to keep this patch > smaller and focused. I can create a jira to track that if you > agree. I will look how difficult adding decimal min-max filters is > tomorrow. Sounds good. I think the biggest thing now is just making sure we've tested everything well enough. For example, you could add tests for insert/update/delete with decimal values that are outside the range for the precision/scale of the column, creating a table with a decimal primary key, etc. http://gerrit.cloudera.org:8080/#/c/9306/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9306/2//COMMIT_MSG@10 PS2, Line 10: We generally include a "Testing" section here, eg. what tests were added, what did you run, etc. You should also take a look at tests/query_tests/test_decimal_queries.py. You should be able to add Kudu to TestDecimalQueries with something like: "v.get_value('table_format').file_format in ['parquet', 'kudu']" You'll also need to take a look at testdata/datasets/functional/schema_constraints.csv to add the decimal related tables to data load for Kudu. http://gerrit.cloudera.org:8080/#/c/9306/1/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: http://gerrit.cloudera.org:8080/#/c/9306/1/be/src/exec/kudu-util.h@a69 PS1, Line 69: > Since it's not used I don't see a reason to keep it around. Well its useful for debugging http://gerrit.cloudera.org:8080/#/c/9306/2/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: http://gerrit.cloudera.org:8080/#/c/9306/2/be/src/exec/kudu-util.cc@214 PS2, Line 214: return ColumnType(PrimitiveType::TYPE_DECIMAL); I imagine the formatter told you to do this. We don't follow it 100%, so you can just follow the example of the above lines. http://gerrit.cloudera.org:8080/#/c/9306/2/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/9306/2/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@625 PS2, Line 625: // TODO: Support Kudu Decimal Min/Max Filters I filed IMPALA-6533. Can you reference that here? http://gerrit.cloudera.org:8080/#/c/9306/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: http://gerrit.cloudera.org:8080/#/c/9306/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@400 PS2, Line 400: return org.apache.kudu.Type.DECIMAL; Another place you can ignore the formatter. -- To view, visit http://gerrit.cloudera.org:8080/9306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f927fce25dc4fa9529a4e0b688825699f5c0ea6 Gerrit-Change-Number: 9306 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Fri, 16 Feb 2018 04:02:49 +0000 Gerrit-HasComments: Yes
