-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix property graph case-sentisitivity #188
Conversation
@@ -33,7 +34,7 @@ class DuckPGQState : public ClientContextState { | |||
// unnamed graph tables | |||
|
|||
//! Property graphs that are registered | |||
std::unordered_map<string, unique_ptr<CreateInfo>> registered_property_graphs; | |||
case_insensitive_map_t<unique_ptr<CreateInfo>> registered_property_graphs; |
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.
This is the only change, all others are made by linters via make format
.
@@ -107,6 +107,7 @@ void DuckPGQState::PopulateEdgeSpecificFields(unique_ptr<DataChunk> &chunk, | |||
void DuckPGQState::ExtractListValues(const Value &list_value, | |||
vector<string> &output) { | |||
auto children = ListValue::GetChildren(list_value); | |||
output.reserve(output.size() + children.size()); |
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.
Reserve memory before emplace.
statement ok | ||
-SELECT count(id) | ||
FROM | ||
GRAPH_TABLE (PG |
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 property graph created at L43 is of lower case.
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.
Just to double check, could you change the test to verify it has the right result? Instead of only checking that the query doesn't crash :)
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 updated the PR description to reflect the manual test result, which looks ok to me.
Let me check how to leverage the testing framework to verify the output as well.
I feel confused at the first place, since other SELECT
statements are also annotated as statement ok
.
duckpgq-extension/test/sql/get_csr_ptr.test
Lines 59 to 60 in f49246d
statement ok | |
SELECT * FROM get_csr_ptr(0); |
Let me check and get back to you. :)
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.
Updated! PTAL, thanks!
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.
Thanks! LGTM
Thanks for the PR! I wasn't aware this would be such a minor change haha Regarding the formatting: there is a PR ongoing to add formatting to the CI. Once that's in place we'll focus more on making sure the formatting is correct. duckdb/extension-ci-tools#121 |
Thank you for the quick review, and giving me the opportunity! And apologize for the latency (was working on something else) :(
Yeah I'm aware of it (last time you pointed me to your formatting CI PR), looking forward to the integration! |
I appreciate your contribution very much, even if it was a minor fix. Don't worry about the latency! Just going to wait for the CI to finish before merging this :) |
Fix issue: #176
Tables names are case-insensitive. Example SQL to display:
This PR uses case-intensitive map to store registered property graphs.
Test result: