Uh oh!
There was an error while loading. Please reload this page.
[Enhancement] Support some spatial functions - #48695
Conversation
…_Touches function and test (apache#48203)
Thearas
commented
Mar 5, 2025
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
doris-robot
commented
Mar 6, 2025
TeamCity cloud ut coverage result: |
doris-robot
commented
Mar 6, 2025
TPC-H: Total hot run time: 32854 ms |
doris-robot
commented
Mar 6, 2025
TPC-DS: Total hot run time: 193344 ms |
doris-robot
commented
Mar 6, 2025
ClickBench: Total hot run time: 30.75 s |
hello-stephen
commented
Mar 6, 2025
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
| if (shapes[i] == nullptr) { | ||
| null_map[row] = 1; | ||
| res->insert_default(); | ||
| break; |
There was a problem hiding this comment.
here better directly return, not need check if (i == 2)
There was a problem hiding this comment.
Sorry, I didn't understand the purpose of checking if i is equal to 2. Checking if i is 2 is actually checking for the presence of a nullptr, I've fixed it now.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| const S2Point& p = loop->vertex(j); | ||
| S2Point closest_point = line->polyline()->Project(p, &next); | ||
| S1Angle distance(closest_point, p); | ||
| if (distance.radians() < 1e-2) { |
There was a problem hiding this comment.
why use this as an error limit? It seems too great
There was a problem hiding this comment.
btw, change it with the constexpr variable
| bool GeoPolygon::intersects(const GeoShape* rhs) const { | ||
| switch (rhs->type()) { | ||
| case GEO_SHAPE_POINT: { | ||
| const GeoPoint* point = (const GeoPoint*)rhs; |
There was a problem hiding this comment.
why do C-style cast here? seems useless
There was a problem hiding this comment.
I need to get to the point variable in the subclass to determine if it's Intersects or not.
There was a problem hiding this comment.
sorry, I mistook the type of the rhs. but here you should use assert_cast
| implements BinaryExpression, ExplicitlyCastableSignature, AlwaysNullable, PropagateNullLiteral { | ||
| public static final List<FunctionSignature> SIGNATURES = ImmutableList.of( | ||
| FunctionSignature.ret(BooleanType.INSTANCE).args(VarcharType.SYSTEM_DEFAULT, VarcharType.SYSTEM_DEFAULT) |
There was a problem hiding this comment.
add StringType for args also
Uh oh!
There was an error while loading. Please reload this page.
| std::unique_ptr<s2builderutil::S2PolygonLayer> layer( | ||
| new s2builderutil::S2PolygonLayer(&result)); | ||
| S2BooleanOperation op(S2BooleanOperation::OpType::INTERSECTION, std::move(layer)); | ||
| S2Error error; |
There was a problem hiding this comment.
deal with the potiential error
| res->insert_data(const_cast<const char*>((char*)&contains_value), 0); | ||
| } | ||
| auto relation_value = Func::evaluate(shapes[0].get(), shapes[1].get()); | ||
| res->insert_data(const_cast<const char*>((char*)&relation_value), 0); |
There was a problem hiding this comment.
- why do these cast? seems weird.
- dont use
insert_datainterface. even if we allocated enough memory, it needs to do an unnecessary check for length. just get the Container(PODArray) from the column and change its data directly.
| static void loop_do(StringRef& lhs_value, StringRef& rhs_value, | ||
| std::vector<std::shared_ptr<GeoShape>>& shapes, int& i, | ||
| std::vector<std::shared_ptr<GeoShape>>& shapes, |
There was a problem hiding this comment.
shapes seems could be unique_ptr. could you confirm and fix it by the way?
…ive number scenarios (apache#48203)
There was a problem hiding this comment.
no need to modify this file anymore since it's not useful now
| return false; | ||
| } | ||
| bool IsSegmentsIntersect(const S2Point& point1, const S2Point& point2, const S2Point& line_point1, |
There was a problem hiding this comment.
all non-const variable or function should be named with underscore naming convention.
There was a problem hiding this comment.
all cases just add in nereids_p0 is enough. but make sure all type-pair's all relationship are covered in this file
There was a problem hiding this comment.
The regression test now contains all relationships for all type pairs. Do I need to add more corner cases?
…re not precise enough and follows the underscore naming convention
doris-robot
commented
Apr 4, 2025
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
PR approved by at least one committer and no changes requested. |
Support for ST_Intersects, ST_Disjoint, ST_Touches sql functions.
Support for ST_Intersects, ST_Disjoint, ST_Touches sql functions.
Support for ST_Intersects, ST_Disjoint, ST_Touches sql functions.
Support for ST_Intersects, ST_Disjoint, ST_Touches sql functions.
Support for ST_Intersects, ST_Disjoint, ST_Touches sql functions.
What problem does this PR solve?
Issue Number: close#48203
Problem Summary:
Support for ST_Intersects, ST_Disjoint, ST_Touches sql functions.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)