You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/04/19 05:18:12 UTC
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10114
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
KUDU-2412: Fix python client compilation in el6 environments
Identifies if the compiler is gcc < 4.6.0 and uses cython
preprocessors to ignore any code using __int128. This
includes test code.
This is a bit of a hacky approach but I needed to
duplicate the entire cdef anytime I used a preprocessor
to remove methods. The preprocessor statements
do not work inside of a cdef stament and you can’t
break out cdef staments to use the additively.
The cython preprocessors do not have a lot of
documentation. Perhaps there is a better approach I missed
that we could apply in a follow up.
See here for documentation:
http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements
Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
---
M python/.gitignore
M python/kudu/__init__.py
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M python/kudu/schema.pyx
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
M python/kudu/tests/util.py
M python/setup.py
9 files changed, 401 insertions(+), 159 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/10114/1
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/client.pyx@1338
PS5, Line 1338: raise KuduException("The decimal type is not supported when GGC version is < 4.6.0"%self)
> Nit: raise Foo("..." % self)
Done
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/client.pyx@2504
PS5, Line 2504: raise KuduException("The decimal type is not supported when GGC version is < 4.6.0"%self)
> Same here.
Done
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/schema.pyx
File python/kudu/schema.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/schema.pyx@772
PS5, Line 772: raise RuntimeError("The decimal type is not supported when GGC version is < 4.6.0"%self)
> Nit: why RuntimeError here and not KuduException?
Done
http://gerrit.cloudera.org:8080/#/c/10114/5/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/5/python/setup.py@27
PS5, Line 27: import distutils.ccompiler
> Unused?
Done
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Apr 2018 21:20:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/10114/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10114/3//COMMIT_MSG@9
PS3, Line 9: This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED from the C++ headers
> Should probably add the bit about config.pxi and how it works.
Done
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/client.pyx@2499
PS3, Line 2499: check_status(self.row.SetUnixTimeMicros(i, <int64_t>
> Still outstanding?
Done
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/client.pyx@2499
PS3, Line 2499: check_status(self.row.SetUnixTimeMicros(i, <int64_t>
> Nit: remove blank line
Done
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/config.pxi.in
File python/kudu/config.pxi.in:
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/config.pxi.in@18
PS3, Line 18: // This file is processed by setup.py
: // See generate_config_pxi.
> Nit: config.pxi is intended to be semi-generic going forward, so perhaps th
Done
http://gerrit.cloudera.org:8080/#/c/10114/2/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/2/python/setup.py@90
PS2, Line 90: src = os.path.join(setup_dir, 'kudu/config.pxi.in')
> Why "cc -x c++" and not "c++"? Thing is we probably want to honor the CC/CX
Done
http://gerrit.cloudera.org:8080/#/c/10114/4/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/4/python/setup.py@70
PS4, Line 70: for x in ['kudu/client.cpp',
> Nit: alphabetic sorting?
Done
http://gerrit.cloudera.org:8080/#/c/10114/4/python/setup.py@93
PS4, Line 93: cc = os.getenv("CXX","cc")
> CXX refers to C++ but 'cc' is the C compiler. So either 'cc' should be 'c++
Done
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Apr 2018 21:07:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Apr 2018 21:25:07 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10114/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10114/3//COMMIT_MSG@9
PS3, Line 9: This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED
Should probably add the bit about config.pxi and how it works.
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/config.pxi.in
File python/kudu/config.pxi.in:
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/config.pxi.in@18
PS3, Line 18: // This file is processed by setup.py using the C preprocessor
: // after defining the macros from kudu/util/int128.h.
Nit: config.pxi is intended to be semi-generic going forward, so perhaps this definition should be more abstract?
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/libkudu_client.pxd
File python/kudu/libkudu_client.pxd:
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/libkudu_client.pxd@20
PS3, Line 20: include "config.pxi"
Not needed anymore? Or just for consistency?
http://gerrit.cloudera.org:8080/#/c/10114/2/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/2/python/setup.py@90
PS2, Line 90: dst_tmp = dst + '.tmp'
Why "cc -x c++" and not "c++"? Thing is we probably want to honor the CC/CXX environment variables, but since we're compiling C++ it's probably better to just consider CXX.
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:00:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/client.pyx@2499
PS3, Line 2499: check_status(self.row.SetUnixTimeMicros(i, <int64_t>
> Nit: remove blank line
Still outstanding?
http://gerrit.cloudera.org:8080/#/c/10114/4/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/4/python/setup.py@70
PS4, Line 70: for x in ['kudu/client.cpp',
Nit: alphabetic sorting?
http://gerrit.cloudera.org:8080/#/c/10114/4/python/setup.py@93
PS4, Line 93: cc = os.getenv("CXX","cc")
CXX refers to C++ but 'cc' is the C compiler. So either 'cc' should be 'c++', or CXX should be CC (probably the latter).
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Apr 2018 21:02:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has uploaded a new patch set (#2) to the change originally created by Grant Henke. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
KUDU-2412: Fix python client compilation in el6 environments
This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED
from the C++ headers and defines a Cython macro PYKUDU_INT128_SUPPORTED.
This is then used to conditionally generate the bits of Python code
that depend on int128-related methods.
See here for documentation:
http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements
Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
---
M python/.gitignore
M python/kudu/__init__.py
M python/kudu/client.pyx
A python/kudu/config.pxi.in
M python/kudu/libkudu_client.pxd
M python/kudu/schema.pyx
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
M python/kudu/tests/util.py
M python/setup.py
10 files changed, 122 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/10114/2
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Apr 2018 22:07:25 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10114
to look at the new patch set (#6).
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
KUDU-2412: Fix python client compilation in el6 environments
This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED from the C++ headers
and defines a Cython macro PYKUDU_INT128_SUPPORTED.
PYKUDU_INT128_SUPPORTED is generated by
replacing the KUDU_INT128_SUPPORTED placeholder
in config.pxi.in to generate config.pxi which is then included
in client.pyx.
This is then used to conditionally generate the bits of Python code
that depend on int128-related methods.
See here for documentation:
http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements
Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
---
M python/.gitignore
M python/kudu/__init__.py
M python/kudu/client.pyx
A python/kudu/config.pxi.in
M python/kudu/libkudu_client.pxd
M python/kudu/schema.pyx
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
M python/kudu/tests/util.py
M python/setup.py
10 files changed, 127 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/10114/6
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 2:
(1 comment)
Grant, you want to finish this up or shall I?
http://gerrit.cloudera.org:8080/#/c/10114/2/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/2/python/setup.py@90
PS2, Line 90: subprocess.check_call(["cc", "-x", "c++", "-o", dst,
> Why "cc -x c++" and not "c++"? Thing is we probably want to honor the CC/CX
because of the funny extension we have to use -x -- otherwise it doesn't know it's a source file and thinks it's a library or something.
Agreed though we should use CXX. At first I attempted to use distutils.ccompiler but it was pretty finicky.
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:04:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/client.pyx@1338
PS5, Line 1338: raise KuduException("The decimal type is not supported when GGC version is < 4.6.0"%self)
Nit: raise Foo("..." % self)
(note the spaces surrounding the percent sign)
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/client.pyx@2504
PS5, Line 2504: raise KuduException("The decimal type is not supported when GGC version is < 4.6.0"%self)
Same here.
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/schema.pyx
File python/kudu/schema.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/5/python/kudu/schema.pyx@772
PS5, Line 772: raise RuntimeError("The decimal type is not supported when GGC version is < 4.6.0"%self)
Nit: why RuntimeError here and not KuduException?
Also note the spaces around the percent sign here.
http://gerrit.cloudera.org:8080/#/c/10114/5/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/5/python/setup.py@27
PS5, Line 27: import distutils.ccompiler
Unused?
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Apr 2018 21:14:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10114/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10114/1//COMMIT_MSG@16
PS1, Line 16: stament
Nit: statement
http://gerrit.cloudera.org:8080/#/c/10114/1//COMMIT_MSG@15
PS1, Line 15: The preprocessor statements
: do not work inside of a cdef stament
This doesn't seem right though because in your patch you ARE using conditional statements inside "cdef class foo" style cdef statements. So is the restriction about using them within "cdef foo(int a, double b)" style statements? I don't see anything in the docs to suggest that this is the case.
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 18:10:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
KUDU-2412: Fix python client compilation in el6 environments
This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED from the C++ headers
and defines a Cython macro PYKUDU_INT128_SUPPORTED.
PYKUDU_INT128_SUPPORTED is generated by
replacing the KUDU_INT128_SUPPORTED placeholder
in config.pxi.in to generate config.pxi which is then included
in client.pyx.
This is then used to conditionally generate the bits of Python code
that depend on int128-related methods.
See here for documentation:
http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements
Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Reviewed-on: http://gerrit.cloudera.org:8080/10114
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Grant Henke <gr...@apache.org>
---
M python/.gitignore
M python/kudu/__init__.py
M python/kudu/client.pyx
A python/kudu/config.pxi.in
M python/kudu/libkudu_client.pxd
M python/kudu/schema.pyx
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
M python/kudu/tests/util.py
M python/setup.py
10 files changed, 127 insertions(+), 34 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Grant Henke: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10114
to look at the new patch set (#5).
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
KUDU-2412: Fix python client compilation in el6 environments
This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED from the C++ headers
and defines a Cython macro PYKUDU_INT128_SUPPORTED.
PYKUDU_INT128_SUPPORTED is generated by
replacing the KUDU_INT128_SUPPORTED placeholder
in config.pxi.in to generate config.pxi which is then included
in client.pyx.
This is then used to conditionally generate the bits of Python code
that depend on int128-related methods.
See here for documentation:
http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements
Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
---
M python/.gitignore
M python/kudu/__init__.py
M python/kudu/client.pyx
A python/kudu/config.pxi.in
M python/kudu/libkudu_client.pxd
M python/kudu/schema.pyx
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
M python/kudu/tests/util.py
M python/setup.py
10 files changed, 128 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/10114/5
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10114
to look at the new patch set (#4).
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
KUDU-2412: Fix python client compilation in el6 environments
This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED from the C++ headers
and defines a Cython macro PYKUDU_INT128_SUPPORTED.
PYKUDU_INT128_SUPPORTED is generated by
replacing the KUDU_INT128_SUPPORTED placeholder
in config.pxi.in to generate config.pxi which is then included
in client.pyx.
This is then used to conditionally generate the bits of Python code
that depend on int128-related methods.
See here for documentation:
http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements
Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
---
M python/.gitignore
M python/kudu/__init__.py
M python/kudu/client.pyx
A python/kudu/config.pxi.in
M python/kudu/libkudu_client.pxd
M python/kudu/schema.pyx
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
M python/kudu/tests/util.py
M python/setup.py
10 files changed, 129 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/10114/4
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/client.pyx@2499
PS3, Line 2499: elif t == KUDU_UNIXTIME_MICROS:
Nit: remove blank line
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/libkudu_client.pxd
File python/kudu/libkudu_client.pxd:
http://gerrit.cloudera.org:8080/#/c/10114/3/python/kudu/libkudu_client.pxd@20
PS3, Line 20: include "config.pxi"
This shouldn't be needed anymore.
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:02:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/client.pyx@1332
PS1, Line 1332: IF KUDU_INT128_SUPPORTED == 1:
> Hmm, so what happens if you reformat this like so:
I did this too as I played with it
http://gerrit.cloudera.org:8080/#/c/10114/1/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/1/python/setup.py@76
PS1, Line 76: # Identify the cc version used and check for __int128 support
> How about using a compile test instead? Then you won't have to worry about
I ended up doing something akin to this in r3
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:01:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/client.pyx@1332
PS1, Line 1332: IF KUDU_INT128_SUPPORTED == 1:
Hmm, so what happens if you reformat this like so:
cdef inline __get_unscaled_decimal(self, int i):
IF KUDU_INT128_SUPPORTED == 1:
cdef int128_t val
check_status(self.row.GetUnscaledDecimal(i, &val))
return val
ELSE:
raise KuduException("The decimal type is not supported when GGC version is < 4.6.0"%self)
The Cython docs seem pretty convinced that IF/ELSE can go anywhere. See http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements:
An IF statement can appear anywhere that a normal statement or declaration can appear, and it can contain any statements or declarations that would be valid in that context, including DEF statements and other IF statements.
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/client.pyx@1339
PS1, Line 1339: raise KuduException("The decimal type is not supported when GGC version is < 4.6.0"%self)
Nit: reformat as:
raise KuduException("The decimal type is not supported when GGC version is < 4.6.0" % self)
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/client.pyx@2500
PS1, Line 2500:
Nit: revert this?
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/client.pyx@2508
PS1, Line 2508: raise KuduException("The decimal type is not supported when GGC version is < 4.6.0"%self)
Nit: see above reformat suggestion.
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/schema.pyx
File python/kudu/schema.pyx:
http://gerrit.cloudera.org:8080/#/c/10114/1/python/kudu/schema.pyx@772
PS1, Line 772: raise RuntimeError("The decimal type is not supported when GGC version is < 4.6.0"%self)
Nit: see earlier reformat suggestion
http://gerrit.cloudera.org:8080/#/c/10114/1/python/setup.py
File python/setup.py:
http://gerrit.cloudera.org:8080/#/c/10114/1/python/setup.py@69
PS1, Line 69: for x in ['kudu/client.cpp', 'kudu/schema.cpp', 'kudu/errors.cpp',
Nit: reformat this to place each file on its own line. Makes it easier to deal with future changes.
http://gerrit.cloudera.org:8080/#/c/10114/1/python/setup.py@76
PS1, Line 76: # Identify the cc version used and check for __int128 support
How about using a compile test instead? Then you won't have to worry about the type of compiler at all; just that you were able to compile something that tried to, say, declare a variable with __int128 type. See thirdparty/preflight.py for efficient examples of this pattern.
After doing that, also update the various exceptions to drop the specific version mentioned in them and just say "your compiler doesn't support the __int128 type" or some such.
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 18:08:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
Patch Set 3:
tweaked this a bit:
- seems that it's fine to list the int128 methods in the .pxd files even if int128 isn't supported. The pxds don't generate their own definitions in the generated C code, they just expose metadata about what might be there, so it doesn't produce syntax errors.
- switched to using the C preprocessor to generate config.pxi.in based on the version checking logic in the client header, to avoid duplicating logic
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 18:59:14 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2412: Fix python client compilation in el6 environments
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has uploaded a new patch set (#3) to the change originally created by Grant Henke. ( http://gerrit.cloudera.org:8080/10114 )
Change subject: KUDU-2412: Fix python client compilation in el6 environments
......................................................................
KUDU-2412: Fix python client compilation in el6 environments
This uses the C preprocessor to check the value of KUDU_INT128_SUPPORTED
from the C++ headers and defines a Cython macro PYKUDU_INT128_SUPPORTED.
This is then used to conditionally generate the bits of Python code
that depend on int128-related methods.
See here for documentation:
http://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-statements
Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
---
M python/.gitignore
M python/kudu/__init__.py
M python/kudu/client.pyx
A python/kudu/config.pxi.in
M python/kudu/libkudu_client.pxd
M python/kudu/schema.pyx
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
M python/kudu/tests/util.py
M python/setup.py
10 files changed, 128 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/10114/3
--
To view, visit http://gerrit.cloudera.org:8080/10114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd93b57020b80597baae9c8d3e0434c46f7fc3d7
Gerrit-Change-Number: 10114
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>