Skip to content

[CALCITE-5280] Add geometry aggregate functions (ST_Accum, ST_Collect, and ST_Union)#2989

Merged
bchapuis merged 1 commit intoapache:mainfrom
bchapuis:5280-geometry-aggregate-functions
Dec 12, 2022
Merged

[CALCITE-5280] Add geometry aggregate functions (ST_Accum, ST_Collect, and ST_Union)#2989
bchapuis merged 1 commit intoapache:mainfrom
bchapuis:5280-geometry-aggregate-functions

Conversation

@bchapuis
Copy link
Member

@bchapuis bchapuis commented Nov 28, 2022

Here, the main point of caution is the creation of the operator table in SqlLibraryOperatorTableFactory.

@bchapuis bchapuis force-pushed the 5280-geometry-aggregate-functions branch from ffdda79 to a3a7777 Compare November 28, 2022 23:09
@rubenada rubenada changed the title [Calcite-5280] Add geometry aggregate functions (ST_Accum, ST_Collect, and ST_Union) [CALCITE-5280] Add geometry aggregate functions (ST_Accum, ST_Collect, and ST_Union) Nov 29, 2022
@bchapuis bchapuis force-pushed the 5280-geometry-aggregate-functions branch 3 times, most recently from dca5c62 to 05f66fe Compare November 29, 2022 23:34
@bchapuis bchapuis marked this pull request as ready for review November 30, 2022 07:22
@bchapuis bchapuis force-pushed the 5280-geometry-aggregate-functions branch from 05f66fe to ebbdb5d Compare December 1, 2022 17:54
import org.apache.calcite.prepare.CalciteCatalogReader;
import org.apache.calcite.runtime.SpatialTypeFunctions;
import org.apache.calcite.schema.Function;
import org.apache.calcite.schema.SchemaPlus;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The large number of new imports is an indication that the code to define the aggregate functions should be somewhere else.

Copy link
Member Author

@bchapuis bchapuis Dec 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this code should have been placed in SqlOperatorTables.createSpatial. However, as it does not address the number of imports, I looked at the SqlStdOperatorTable and wondered if we should create a SqlSpatialOperatorTable. I will probably dig into this a bit, but please stop me if it is a bad idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julianhyde I moved this part of the code to a new class called SqlSpatialTypeOperatorTable, which implements the SqlOperatorTable interface. I also experimented with the ReflectiveSqlOperatorTable, but I believe that using this approach would require declaring properties of type SqlUserDefinedAggFunction for each AggregateFunction. What do you think about continuing down this path?

@bchapuis bchapuis force-pushed the 5280-geometry-aggregate-functions branch from cef9e87 to 1c78de0 Compare December 12, 2022 21:17
@bchapuis bchapuis force-pushed the 5280-geometry-aggregate-functions branch from 1c78de0 to 5fefdd8 Compare December 12, 2022 21:26
@bchapuis bchapuis merged commit de89d7a into apache:main Dec 12, 2022
@bchapuis bchapuis deleted the 5280-geometry-aggregate-functions branch December 12, 2022 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants