Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/23604 )
Change subject: IMPALA-13263: Add single-argument overload for ST_ConvexHull() and tests ...................................................................... Patch Set 4: (8 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/23604/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23604/4//COMMIT_MSG@7 PS4, Line 7: and tests I don't think that mentioning tests is useful - all commits should have corresponding tests :) http://gerrit.cloudera.org:8080/#/c/23604/4//COMMIT_MSG@9 PS4, Line 9: to : support additional geometric input types. This improves usability by : allowing the function to be called directly with different geometry : representations without explicit conversion. This looks a bit unnecessarily verbose to me. The single argument overload is the most important one, multi argument ones are probably used just to simplify some expressions in the original ESRI UDFs. PostGIS seems to have only the single argument version https://postgis.net/docs/ST_ConvexHull.html http://gerrit.cloudera.org:8080/#/c/23604/4//COMMIT_MSG@16 PS4, Line 16: ST_ConvexHull Can you mention that there was no tests at all for ST_ConvexHull()? http://gerrit.cloudera.org:8080/#/c/23604/4//COMMIT_MSG@17 PS4, Line 17: geometric nit: I have always seen this written as "geometry types", not geometric. http://gerrit.cloudera.org:8080/#/c/23604/4/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test File testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test: http://gerrit.cloudera.org:8080/#/c/23604/4/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@2723 PS4, Line 2723: MULTIPOLYGON Hmm, this is not related to this change, but I am surprised that the result returned is multipolygon instead of a polygon. https://postgis.net/docs/ST_ConvexHull.html http://gerrit.cloudera.org:8080/#/c/23604/4/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@2727 PS4, Line 2727: select ST_AsText(ST_ConvexHull(ST_GeomFromText('multipoint ((0 0), (2 0), (2 2), (0 2), (1 1))'))); Can you add a similar test with other types, e.g. polygon? The point set used here looks a valid both as polygon and as linestring http://gerrit.cloudera.org:8080/#/c/23604/4/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@2745 PS4, Line 2745: select ST_ConvexHull('invalid geometry'); Can you add a test for a geometry where the convex hull is not valid polygon, e.g. the input is a single point? http://gerrit.cloudera.org:8080/#/c/23604/4/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@2756 PS4, Line 2756: # Test ST_ConvexHull with too many arguments (more than 8) Can you add a positive test with multiple input geometries? While the ticket is about the missing single argument overload, there is not test currently for any of the overloads. -- To view, visit http://gerrit.cloudera.org:8080/23604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb17d98f5e75929ec0143aa16195a84dd6e50796 Gerrit-Change-Number: 23604 Gerrit-PatchSet: 4 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Tue, 11 Nov 2025 19:55:24 +0000 Gerrit-HasComments: Yes
