Conversation
--------- Co-authored-by: treyfel <58570309+treyfel@users.noreply.github.com> Co-authored-by: maxspeer <82819900+MaxSpeer@users.noreply.github.com>
|
Hi thanks for your contribution, |
Hi @mspruc, the print statements we introduced are only in benchmark classes and in tests. We think this is fine. Could you please point to the statements that should be removed? |
| import org.apache.wayang.core.plan.wayangplan.BinaryToUnaryOperator; | ||
| import org.apache.wayang.core.types.DataSetType; | ||
|
|
||
| public class SpatialJoinOperator<InputType0, InputType1> extends BinaryToUnaryOperator<InputType0, InputType1, Tuple2<InputType0, InputType1>> { |
There was a problem hiding this comment.
Add a comment about the operator similar to what you had done to spatialfilter operator
| filterTasks.add(filterOperator); | ||
| } else if (nextTask.getOperator() instanceof JdbcProjectionOperator projectionOperator) { | ||
| final var operator = nextTask.getOperator(); | ||
| if (operator instanceof JdbcFilterOperator || operator instanceof SpatialFilterOperator) { |
There was a problem hiding this comment.
Shouldn't this be a jdbcspatialfilter operator? Why referencing the logical operator?
There was a problem hiding this comment.
And maybe then we do not need the extra condition in the if?
There was a problem hiding this comment.
The JdbcSpatialOperator is located in the Spatial Plugin and we don't want to introduce this dependency. The findJdbcExecutionOperatorTaskInStage() function ensures that only Jdbc Operators are selected. For consistency we could change the other JDBC operators to their wayang operators (e.g. JdbcFilterOperator -> FilterOperator)
There was a problem hiding this comment.
Yes, I think having the Wayang operators is a good idea. In any case, we wouldn't have an execution operator from another platform here, so it's more about the type of operator and not about its execution platform.
So, if we say eg. FilterOperator, would that also catch the case for the SpatialFilterOperator? It should, right?
| } else if (nextTask.getOperator() instanceof JdbcJoinOperator joinOperator) { | ||
| joinTasks.add(joinOperator); | ||
| projectionTask = (JdbcProjectionOperator) operator; | ||
| } else if (operator instanceof JdbcJoinOperator || (operator instanceof SpatialJoinOperator)) { |
There was a problem hiding this comment.
same here? why a special case for the spatial wayang join operator is needed?
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| @Disabled("Requires local Postgres test database.") |
There was a problem hiding this comment.
can you check if the hsqldb library we use for other jdbc tests supports spatial operators? Then we can have the tests without requiring a postgres instance setup and running.
There was a problem hiding this comment.
hsqldb does not support spatial operations. In JdbcSpatialFilterOperatorTest we do however test if the generated Spatial SQL Query is correct.
|
And regarding the print outs, I agree with @mspruc. You can either remove them or you can homogenize them as some benchmarks print on the top and some others printout the count. But that's more of a minor. |
|
We updated the spatial benchmarks with consistent print statements across the different classes. The benchmark scripts serve as examples on how to use the spatial operations and we use them to compare operator performance on different platforms. |
5 participants
Hi everyone,
together with @zkaoudi we worked on extending wayang with spatial operations.
Please consider our PR. We are looking forward to your feedback.
Summary
This PR introduces spatial processing capabilities in Wayang by adding two new logical operators: SpatialFilter and SpatialJoin, with platform-specific implementations for Java, Spark, and PostgreSQL/JDBC.
The goal is to enable spatial filter and join operations end-to-end across Wayang’s Java, Spark, and Postgres execution paths and remain extendable for other spatial operations.
What Changed
Design Choices
Platform Implementation Notes
Testing