-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Enhancement] Support some spatial functions #48695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…_Touches function and test (apache#48203)
…T_touches function (apache#48203)
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
TeamCity cloud ut coverage result: |
TPC-H: Total hot run time: 32854 ms |
TPC-DS: Total hot run time: 193344 ms |
ClickBench: Total hot run time: 30.75 s |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here better directly return, not need check if (i == 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
be/src/geo/geo_types.cpp
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use this as an error limit? It seems too great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, change it with the constexpr variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
be/src/geo/geo_types.cpp
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do C-style cast here? seems useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I mistook the type of the rhs. but here you should use assert_cast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add StringType for args also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
be/src/geo/geo_types.cpp
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deal with the potiential error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shapes seems could be unique_ptr. could you confirm and fix it by the way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…ive number scenarios (apache#48203)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to modify this file anymore since it's not useful now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
be/src/geo/geo_types.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
| bool IsSegmentsIntersect(const S2Point& point1, const S2Point& point2, const S2Point& line_point1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all non-const variable or function should be named with underscore naming convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
TPC-H: Total hot run time: 34147 ms |
TPC-DS: Total hot run time: 192317 ms |
ClickBench: Total hot run time: 30.69 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
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.
…functions (#50073) pick: #37003, #48695 and #49665 --------- Co-authored-by: Mryange <[email protected]> Co-authored-by: koi <[email protected]>
…0061) pick: #48695 and #49665 --------- Co-authored-by: koi <[email protected]>
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)