[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 11: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 11 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 25 Sep 2020 14:02:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - Added a sanity test that checks that numbers are evenly distributed when inserted into a Kudu table. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Reviewed-on: http://gerrit.cloudera.org:8080/16419 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h M tests/common/kudu_test_suite.py M tests/query_test/test_kudu.py 15 files changed, 519 insertions(+), 200 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 12 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6474/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 11 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 25 Sep 2020 08:40:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 11: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/6472/ -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 11 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Sep 2020 19:09:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 11 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Sep 2020 15:11:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6472/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 11 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Sep 2020 15:11:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 10: Code-Review+2 Yeah, mystery solved! -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 10 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Sep 2020 15:06:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7274/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 10 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Sep 2020 15:00:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - Added a sanity test that checks that numbers are evenly distributed when inserted into a Kudu table. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h M tests/common/kudu_test_suite.py M tests/query_test/test_kudu.py 15 files changed, 519 insertions(+), 200 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/10 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 10 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 9: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7272/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 9 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Sep 2020 13:56:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - Added a sanity test that checks that numbers are evenly distributed when inserted into a Kudu table. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h M tests/common/kudu_test_suite.py M tests/query_test/test_kudu.py 15 files changed, 520 insertions(+), 200 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/9 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 9 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 8: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7271/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 8 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 24 Sep 2020 13:26:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - Added a sanity test that checks that numbers are evenly distributed when inserted into a Kudu table. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h M tests/common/kudu_test_suite.py M tests/query_test/test_kudu.py 15 files changed, 520 insertions(+), 200 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/8 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 8 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16419/7/be/CMakeLists.txt
File be/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/16419/7/be/CMakeLists.txt@339
PS7, Line 339: "-isystem${BOOST_INCLUDEDIR}"
> This seems to imply that boost includes are read from the system directorie
-isystem mostly behaves the same as -I but has implications for search order
and warnings - https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html
Although I think the fact that we have -I/usr/include on the search path means
that /usr/include has priority over the boost headers. I think that's creeping
in from OPENSSL_INCLUDE_DIR. Maybe OPENSSL_INCLUDE_DIR and SASL_INCLUDE_DIR
should be -isystem as well.
--
To view, visit http://gerrit.cloudera.org:8080/16419
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df
Gerrit-Change-Number: 16419
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 23 Sep 2020 18:37:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/7/be/CMakeLists.txt File be/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/16419/7/be/CMakeLists.txt@337 PS7, Line 337: KUDU_INLCUDE_DIR I realized that this is a typo - inlcude vs include Probably it was instantiated with an empty string, which messed up compilation somehow. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 23 Sep 2020 18:01:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16419/7/be/CMakeLists.txt
File be/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/16419/7/be/CMakeLists.txt@339
PS7, Line 339: "-isystem${BOOST_INCLUDEDIR}"
This seems to imply that boost includes are read from the system directories.
My best bet at the cause of the build failures are the changes in this file.
--
To view, visit http://gerrit.cloudera.org:8080/16419
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df
Gerrit-Change-Number: 16419
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 23 Sep 2020 16:52:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: I also can't reproduce it locally, but I poked around a little bit. I ran with ninja -v and modified the command that compiles impala-ir.cc to output the preprocessed output. Looks like it's picking up the boost headers from /usr/include not the toolchain. Unclear why. Maybe that's the difference with the build server - it might not have boost installed at the system level, or something like that. d /home/tarmstrong/Impala/impala/be/src/codegen && /opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/llvm-5.0.1-p3/bin/clang++ -emit-llvm -c -std=c++14 -DIR_COMPILE -DHAVE_INTTYPES_H -DHAVE_NETINET_IN_H -DBOOST_DATE_TIME_POSIX_TIME_STD_CONFIG -DBOOST_NO_EXCEPTIONS -DKUDU_HEADERS_NO_STUBS -fcolor-diagnostics -Wno-deprecated -Wno-return-type-c-linkage -fsigned-char -O1 -Werror -DADDRESS_SANITIZER --gcc-toolchain=/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/gcc-7.5.0 -I/home/tarmstrong/Impala/impala/be/src -I/home/tarmstrong/Impala/impala/be/generated-sources -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/thrift-0.9.3-p8/include -I -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/glog-0.3.4-p3/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/gflags-2.2.0-p2/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/gtest-1.6.0/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/rapidjson-1.1.0/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/avro-1.7.4-p5/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/orc-1.6.2-p7/include -I -isystem/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/boost-1.61.0-p2/include -isystem/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/kudu-5ad5d3d66/release/include -I/usr/lib/jvm/java-8-openjdk-amd64/include -I/usr/lib/jvm/java-8-openjdk-amd64/include/linux -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/re2-20190301/include -I/usr/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/bzip2-1.0.6-p2/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/zlib-1.2.8/include -I/usr/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/openldap-2.4.47/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/protobuf-3.5.1/include -I/opt/Impala-Toolchain/toolchain-packages-gcc7.5.0/cctz-2.2/include impala-ir.cc -E -o impala-no-sse-tmp.preproc.cc -E -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 19:03:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7241/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 18:39:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: The issue was in compiling impala-ir.cc, so I think it must be somehow related to the extra files included in there (or their dependencies). But I'm not sure exactly what's going on. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 18:28:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: I see https://jenkins.impala.io/job/ubuntu-16.04-build-only/12441/console 08:24:50 [ 98%] Generating ../../../llvm-ir/impala-sse.bc 08:24:50 In file included from impala-ir.cc:28: 08:24:50 In file included from /home/ubuntu/tmp.vCJfK70Ryk/be/src/codegen/codegen-anyval-ir.cc:19: 08:24:50 In file included from /home/ubuntu/tmp.vCJfK70Ryk/be/src/runtime/string-value.inline.h:22: 08:24:50 In file included from /home/ubuntu/tmp.vCJfK70Ryk/be/src/runtime/string-value.h:26: 08:24:50 [1m/home/ubuntu/tmp.vCJfK70Ryk/be/src/udf/udf.h:26:10: [0m[0;1;31mfatal error: [0m[1m'boost/cstdint.hpp' file not found[0m 08:24:50 #include 08:24:50 [0;1;32m ^~~ 08:24:50 [0mIn file included from impala-ir.cc:28: -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 18:15:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7238/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 15:25:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: Code-Review+2 +2, though I have no idea why the code review check jobs failed. I don't see it on other reviews. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 15:15:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 7: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7234/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 09:20:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - Added a sanity test that checks that numbers are evenly distributed when inserted into a Kudu table. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h M tests/common/kudu_test_suite.py M tests/query_test/test_kudu.py 15 files changed, 520 insertions(+), 200 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/7 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 6: Interesting errors... For example is not found, which should be unrelated... I'll try and rebase it to see if it helps. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 08:58:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 6: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7233/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Sep 2020 08:28:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py@570
PS5, Line 570:
: table_name = "partitioning"
: table_full_name = unique_database + ".partitioning"
: cursor.execute("""CREATE TABLE %s (a INT PRIMARY KEY)
: PARTITION BY HASH(a) PARTITI
> I had the same thought, that we could create a managed kudu table instead.
Done
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py@598
PS5, Line 598:
> optional: This could be moved to common code, e.g. near https://github.com/
I'm not sure this algorithm is general enough. It doesn't deal with indentation
so doesn't handle sections robustly, and within each section it can only find
at most one row. I think it is quite specialised to this use case.
--
To view, visit http://gerrit.cloudera.org:8080/16419
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df
Gerrit-Change-Number: 16419
Gerrit-PatchSet: 6
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 22 Sep 2020 08:17:12 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - Added a sanity test that checks that numbers are evenly distributed when inserted into a Kudu table. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h M tests/common/kudu_test_suite.py M tests/query_test/test_kudu.py 15 files changed, 520 insertions(+), 200 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/6 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr
..
Patch Set 5: Code-Review+1
(1 comment)
I think this addresses my concerns. Csaba, feel free to +2 once you're happy
with the tests - don't wait for me.
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py@570
PS5, Line 570: with self.temp_kudu_table(kudu_client, [INT32],
num_partitions=3) as kudu_table:
: impala_table_name =
self.get_kudu_table_base_name(kudu_table.name)
: props = "TBLPROPERTIES('kudu.table_name'='%s')" %
kudu_table.name
: cursor.execute("CREATE EXTERNAL TABLE %s STORED AS KUDU
%s" % (
: impala_table_name, props))
> Do we need the temp_kudu_table + external kudu table pointing to it, couldn
I had the same thought, that we could create a managed kudu table instead.
--
To view, visit http://gerrit.cloudera.org:8080/16419
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df
Gerrit-Change-Number: 16419
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 22 Sep 2020 03:45:09 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py@570
PS5, Line 570: with self.temp_kudu_table(kudu_client, [INT32],
num_partitions=3) as kudu_table:
: impala_table_name =
self.get_kudu_table_base_name(kudu_table.name)
: props = "TBLPROPERTIES('kudu.table_name'='%s')" %
kudu_table.name
: cursor.execute("CREATE EXTERNAL TABLE %s STORED AS KUDU
%s" % (
: impala_table_name, props))
Do we need the temp_kudu_table + external kudu table pointing to it, couldn't
we just use a unique database, and create a managed Kudu table in it? The
cleanup of unique database would handle the dropping of the table.
Note that I am not too familiar with KuduTestSuite, so maybe there is a reason
why it is done differently here.
http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py@598
PS5, Line 598: def extract_kudu_rows_from_profile(profile):
optional: This could be moved to common code, e.g. near
https://github.com/apache/impala/blob/master/tests/common/test_result_verifier.py#L574
A function that takes two regexps (first to match the section second to match
the line) and a profile seems a generally useful one.
--
To view, visit http://gerrit.cloudera.org:8080/16419
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df
Gerrit-Change-Number: 16419
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 21 Sep 2020 13:19:58 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 5: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7214/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Sep 2020 10:38:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/16419/5/tests/query_test/test_kudu.py@589 PS5, Line 589: flake8: E261 at least two spaces before inline comment -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Sep 2020 10:26:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - Added a sanity test that checks that numbers are evenly distributed when inserted into a Kudu table. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h M tests/common/kudu_test_suite.py M tests/query_test/test_kudu.py 15 files changed, 517 insertions(+), 200 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/5 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG@12 PS3, Line 12: - TODO: I don't think we have good test coverage on this, I tried to > Would it be possible to write a test to call both the non-llvm and the llvm I think it would be simplest if the number of partitions and impalads was the same. That's what I'm assuming about the hash function, yeah. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 17 Sep 2020 03:32:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG@12 PS3, Line 12: - TODO: I don't think we have good test coverage on this, I tried to > Does the number of partitions and the number of impalads have to be the sam Would it be possible to write a test to call both the non-llvm and the llvm-code to hash a set of values. If so, then we can just compare the hash results. For v in values hash1 = non_llvm(v); hash2 = llvm(v); assert(hash1 == hash2); -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 Sep 2020 13:35:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/4/be/src/exec/kudu-util-ir.cc File be/src/exec/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/4/be/src/exec/kudu-util-ir.cc@54 PS4, Line 54: // TODO: codegen this to eliminate branching on type. > I am not sure if this is still relevant - after this change this function i This function is moved here from kudu-util.cc. This change is not the only place that calls this function (see https://github.com/apache/impala/blob/7b44b351321969fa6a90212f4c9b56c521a85ec4/be/src/exec/kudu-table-sink.cc#L264) and the TODO may still apply there. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 Sep 2020 12:51:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 4: Code-Review+1 (1 comment) About testing: I couldn't come up with anything else than Tim's sanity test. I am ok with testing this one manually, but a test like that would be a nice addittion. http://gerrit.cloudera.org:8080/#/c/16419/4/be/src/exec/kudu-util-ir.cc File be/src/exec/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/4/be/src/exec/kudu-util-ir.cc@54 PS4, Line 54: // TODO: codegen this to eliminate branching on type. I am not sure if this is still relevant - after this change this function in called with constant col_type argument in the codegend code, so the llvm optimizer has the chance to remove this branch if it inlines the function. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 Sep 2020 10:30:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG@12 PS3, Line 12: - TODO: I don't think we have good test coverage on this, I tried to > One sanity check I can think of is to write to a hash partitioned table and Does the number of partitions and the number of impalads have to be the same for this to work? Am I right that the assumption here is that the hash function is uniform and if the implementation of partitioning is correct, this uniformity is preserved? -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 Sep 2020 08:23:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 4: (2 comments) Thank you both! http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc File be/src/exec/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@32 PS2, Line 32: DCHECK(success); > That's incorrect, they are removed in release builds (i.e. when NDEBUG is s Okay. Good to know! http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc File be/src/exprs/kudu-partition-expr-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc@40 PS2, Line 40: kudu::client::KuduPartitioner* kudu_partitioner = ctx->partitioner.get(); : DCHECK(kudu_partitioner != nullp > Now I see the point, I thought for some reason you were talking about '*row Done -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 14 Sep 2020 17:52:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7175/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 14 Sep 2020 17:12:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - TODO: I don't think we have good test coverage on this, I tried to fail tests in tests/query_test/test_kudu.py with incorrect implementations but they passed. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 13 files changed, 447 insertions(+), 197 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/4 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 3: (3 comments) Great to see this finally removed. I think we need to reach closure on the testing, and then I'd like to do a final close review of the codegen logic just to make sure it's correct. http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16419/3//COMMIT_MSG@12 PS3, Line 12: - TODO: I don't think we have good test coverage on this, I tried to One sanity check I can think of is to write to a hash partitioned table and check the profile to make sure that the different instances of the writer got similar numbers of rows. E.g. if you insert values [0,] into a hash partitioned table with 3 hash partitions, you could check, if, say each partition got between 3000 and 4000 values inserted into it. http://gerrit.cloudera.org:8080/#/c/16419/3/be/src/exec/kudu-util-ir.cc File be/src/exec/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/3/be/src/exec/kudu-util-ir.cc@54 PS3, Line 54: // TODO: codegen this to eliminate branching on type. I think we can remove this since we use ColumnType::ToIR() and replace the argument with a constant. http://gerrit.cloudera.org:8080/#/c/16419/3/be/src/exprs/kudu-partition-expr.cc File be/src/exprs/kudu-partition-expr.cc: http://gerrit.cloudera.org:8080/#/c/16419/3/be/src/exprs/kudu-partition-expr.cc@157 PS3, Line 157: "class.impala::Status" Can you make this Status::LLVM_CLASS_NAME to fit the current pattern. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 14 Sep 2020 16:30:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc File be/src/exec/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@32 PS2, Line 32: DCHECK(success); > My understanding is that DCHECK is applied even in release code. If success That's incorrect, they are removed in release builds (i.e. when NDEBUG is set). -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 14 Sep 2020 15:37:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Qifan Chen has posted comments on this change. (
http://gerrit.cloudera.org:8080/16419 )
Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr
..
Patch Set 3:
(5 comments)
Thanks a lot for the explanation. Appreciate it.
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc
File be/src/exec/kudu-util-ir.cc:
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@32
PS2, Line 32: DCHECK(success);
> I copied these functions from kudu-util.cc into this new file, it already h
My understanding is that DCHECK is applied even in release code. If success is
false, we will stop here and bail out, right?
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@35
PS2, Line 35: Invalid TimestampValue in function Convert
> You mean the name of this conversion function? I added it to the message.
Yes. The idea is that we can know for sure which function is involved in the
message.
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc
File be/src/exprs/kudu-partition-expr-ir.cc:
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc@40
PS2, Line 40: *row = kudu_row;
: *partitioner = kudu_partitioner;
> Why do you think it's better?
Just to make sure these two pointers are always valid to assign values to.
Maybe they are every time?
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc
File be/src/exprs/kudu-partition-expr.cc:
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@152
PS2, Line 152: lvm::Function* const write_kudu_fn =
: codegen->GetFunction(IRFunction::WRITE_KUDU_VALUE, false);
> I don't quite understand it, could you help me?
I mean people changes the declaration of Impala::Status.
http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@229
PS2, Line 229: tus KuduPartitionExpr::GetCodegendComputeFnImpl(
: LlvmCodeGen* codegen, llvm::Function** fn) {
: llvm::Function* const kudu_partition_row_fn =
: codegen->GetFunction(IRFunction::GET_KUDU_PARTI
> It's not redundant, 'next_eval_child_block' is assigned to 'current_eval_ch
Okay. Thanks for the explanation.
--
To view, visit http://gerrit.cloudera.org:8080/16419
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df
Gerrit-Change-Number: 16419
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 14 Sep 2020 14:07:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 3: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7171/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 14 Sep 2020 09:51:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - TODO: I don't think we have good test coverage on this, I tried to fail tests in tests/query_test/test_kudu.py with incorrect implementations but they passed. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 13 files changed, 443 insertions(+), 197 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/3 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 2: (15 comments) Looks good to me! http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc File be/src/exec/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@32 PS2, Line 32: DCHECK(success); Wonder if we need this. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@35 PS2, Line 35: Invalid TimestampValue: " + tv->ToString() Can we also include the function name in the message? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util-ir.cc@45 PS2, Line 45: Invalid DateValue Same as above. Also, is it possible to include the invalid value itself? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util.h@88 PS2, Line 88: ConvertTimestampValue( Maybe reworded to 'ConvertTimestampValueToKudu()' to make it clear that the result is in Kudu format. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exec/kudu-util.h@91 PS2, Line 91: ConvertDateValue Same as above. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc File be/src/exprs/kudu-partition-expr-ir.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc@40 PS2, Line 40: *row = kudu_row; : *partitioner = kudu_partitioner; Should we DCHEK(row && partitioner) here? http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr-ir.cc@44 PS2, Line 44: KuduPartitionRow nit: reworded as GetKuduPartitionRow(). http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.h File be/src/exprs/kudu-partition-expr.h: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.h@77 PS2, Line 77: static void SetKuduPartialRowAndPartitioner(ScalarExprEvaluator* eval, int fn_ctx_idx, A comment here will be nice. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc File be/src/exprs/kudu-partition-expr.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@122 PS2, Line 122: A comment here will help. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@152 PS2, Line 152: / This can only fail if we set a col to an incorrect type, which would be a bug in : // planning, so we could DCHECK but in codegen code we can't so we do not check it. This could happen when the impala::Status is not a class anymore. Highly unlikely. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@165 PS2, Line 165: A comment here will help. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@175 PS2, Line 175: llvm::Value* args[2]; Maybe add a comment on what these two arguments are for. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@206 PS2, Line 206: non_null_block maybe renamed as not_null_block http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@222 PS2, Line 222: (GetChild(i) Looks like we can directly use child_expr here. http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@229 PS2, Line 229: llvm::BasicBlock* next_eval_child_block = : llvm::BasicBlock::Create(context, "eval_child", function); : builder.CreateBr(next_eval_child_block); : current_eval_child_block = next_eval_child_block; I wonder if this block is redundant when i == num_children - 1. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 10 Sep 2020 19:40:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 2: I realized that this was the last use of GetCodegendComputeFnWrapper(), so it could be removed completely. It has several TODOs, which could be also removed, e.g.: https://github.com/apache/impala/blob/6c8a3dfc339e43a8992af2ff3429ba5940a061ec/be/src/exprs/scalar-expr.h#L141 Do you want to do this here, or it should be a separate patch for IMPALA-7656? -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 10 Sep 2020 10:00:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16419/2//COMMIT_MSG@12 PS2, Line 12: - TODO: I don't think we have good test coverage on this, I tried to I think what's probably going on is that it's not necessary for correctness - you can write the Kudu row via the Kudu client on any node and it gets sent to the correct tserver internally. It's kinda tricky to test cause it does depend on having a Kudu table around to instantiate the expr. It looks like the original patch just checked the performance. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 05 Sep 2020 21:02:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/7108/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 04 Sep 2020 14:20:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16419 ) Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc File be/src/exprs/kudu-partition-expr.cc: http://gerrit.cloudera.org:8080/#/c/16419/2/be/src/exprs/kudu-partition-expr.cc@171 PS2, Line 171: // Function prototype. tab used for whitespace -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 04 Sep 2020 14:09:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10078: Proper codegen for KuduPartitionExpr
Daniel Becker has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16419 Change subject: IMPALA-10078: Proper codegen for KuduPartitionExpr .. IMPALA-10078: Proper codegen for KuduPartitionExpr Implementing codegen for KuduPartitionExpr. Testing: - TODO: I don't think we have good test coverage on this, I tried to fail tests in tests/query_test/test_kudu.py with incorrect implementations but they passed. Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df --- M be/CMakeLists.txt M be/src/codegen/gen_ir_descriptions.py M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/kudu-util-ir.cc M be/src/exec/kudu-util.cc M be/src/exec/kudu-util.h M be/src/exprs/CMakeLists.txt A be/src/exprs/kudu-partition-expr-ir.cc M be/src/exprs/kudu-partition-expr.cc M be/src/exprs/kudu-partition-expr.h 11 files changed, 363 insertions(+), 133 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/16419/2 -- To view, visit http://gerrit.cloudera.org:8080/16419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ifcae34f71b407837e2c5f1b97aa230e490a268df Gerrit-Change-Number: 16419 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Tim Armstrong
