Skip to content

Conversation

mkaruza
Copy link
Contributor

@mkaruza mkaruza commented Sep 24, 2025

Fixes #4536

@mkaruza mkaruza marked this pull request as draft September 24, 2025 10:01
@mkaruza mkaruza force-pushed the mkaruza/github#4536 branch from b9f3a30 to a1a5f6a Compare September 24, 2025 10:02
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

Looks good so far 👍🏻


IndexResult Search(const AstGeoNode& node, string_view active_field) {
DCHECK(!active_field.empty());
double converted_radius = GeoIndex::ConvertToRadiusInMeters(node.radius, node.unit);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make the index do that internally

@mkaruza mkaruza force-pushed the mkaruza/github#4536 branch from e54ccac to 0c89126 Compare September 25, 2025 19:18
dranikpg
dranikpg previously approved these changes Sep 26, 2025
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

Looks valid so far

Comment on lines 48 to 53
struct AstGeoNode {
AstGeoNode(double lat, double lon, size_t radius, std::string unit);
double lat, lon;
size_t radius;
std::string unit;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: parse unit from lexer (so we can throw the error in time)

Comment on lines 638 to 650
double lat;
if (!absl::SimpleAtod(coordinates[0], &lat))
return false;

double lon;
if (!absl::SimpleAtod(coordinates[1], &lon))
return false;

entry index_entry = std::make_pair(point{lat, lon}, id);

rtree_->insert(index_entry);

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just style comment how to reduce volume 😅

// absl::StrSplit can split into a pair but you can't check there easily
double lon, lat;
if (!absl::SimpleAtod() || !absl::SimpleAtod())
  return false;
rtree_->emplace(point{}, id)
return true;

Comment on lines 700 to 711
double lat;
if (!absl::SimpleAtod(coordinates[0], &lat)) {
return std::nullopt;
}

double lon;
if (!absl::SimpleAtod(coordinates[1], &lon)) {
return std::nullopt;
}

return point{lat, lon};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just style comment, feel free to ignore 😅

double lon, lat;
if (!absl::SimpleAtod() || !absl::SimpleAtod())
  return nullopt;

Comment on lines 222 to 224
std::vector<DocId> GetAllDocsWithNonNullValues() const override {
return std::vector<DocId>{};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That need to be for a @geofield:* query? Is this possible? If not, we should explicitly disallow it. Not saying to solve it now, just a TODO would help us not to forget

}
}

std::optional<GeoIndex::point> GeoIndex::GetGeoPoint(const DocumentAccessor& doc,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be in anonymous namespace

Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

Well, you can improve the rules by making proxy objects and making fewer composable rules out of them. I'm sure it can be done. Yet... it will be more code I assume. Not sure, I mean you have to admit it doesn't look really sexy 🙄

@mkaruza mkaruza force-pushed the mkaruza/github#4536 branch 2 times, most recently from 567e33c to 33518a2 Compare September 26, 2025 15:01
@mkaruza mkaruza force-pushed the mkaruza/github#4536 branch from 33518a2 to 8d6c4dc Compare September 26, 2025 15:51
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.

Support GEO search
2 participants