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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 19 Apr 2018 18:08:44 +0000 Gerrit-HasComments: Yes
