Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed case sensitivity on label usage of reserved keyword #1040

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Fixed case sensitivity on label usage of reserved keyword #1040

merged 1 commit into from
Jul 20, 2023

Conversation

panosfol
Copy link
Contributor

@panosfol panosfol commented Jul 15, 2023 •
edited
Loading

This resolves #1028.

@panosfol
Copy link
Contributor Author

@rafsun42 Awaiting for review

@rafsun42
Copy link
Member

@panosfol Could you please add regression tests for any issues that were fixed by this PR?

@panosfol
Copy link
Contributor Author

Yes of course! Want me to add them to here or open a new PR?

@rafsun42
Copy link
Member

rafsun42 commented Jul 17, 2023 •
edited
Loading

@panosfol

Yes of course! Want me to add them to here or open a new PR?

You can add them here. You may need to rebase, squash and force push into your local branch.

@panosfol
Copy link
Contributor Author

OK im on it

@panosfol
Copy link
Contributor Author

@rafsun42 done with adding regression tests

@jrgemignani
Copy link
Contributor

@panosfol FYI: Any addition (feature, modification, bug fix) to AGE is required to have regression tests. The only exceptions are where none are possible.

@panosfol
Copy link
Contributor Author

@panosfol FYI: Any addition (feature, modification, bug fix) to AGE is required to have regression tests. The only exceptions are where none are possible.

Noted!

@rafsun42
Copy link
Member

@panosfol Did you check the other two comments I made above yesterday?

@panosfol
Copy link
Contributor Author

panosfol commented Jul 19, 2023 •
edited
Loading

@panosfol Did you check the other two comments I made above yesterday?

About adding regression tests? I did that using git squash and git push --force.

@panosfol
Fixed case sensitivity on label usage of reserved keyword
c9fca3d 
and added regression tests.

Fixed the case when a label was assigned a name of a
reserved keyword, resulting in the label being lowercase
regardless of the user input.

Signed-off-by: Panagiotis Foliadis<pfoliadis@hotmail.com>
@panosfol
Copy link
Contributor Author

@rafsun42 I looked into the pstrdup() function and its something that postgres uses for every string assignment to avoid memory leaks. Also I added truncate_identifier() to shave off any whitespaces.

@rafsun42 rafsun42 merged commit eeafb0a into apache:master Jul 20, 2023
6 checks passed
@rafsun42
Copy link
Member

@panosfol Good. Send a PR to PG11 and PG12.

This was referenced Jul 21, 2023
Merged
Merged
MuhammadTahaNaveed pushed a commit to MuhammadTahaNaveed/age that referenced this pull request Aug 24, 2023
@panosfol @MuhammadTahaNaveed
Fixed case sensitivity on label usage of reserved keyword (apache#1040)
f48285f 
Fixed the case when a label was assigned a name of a
reserved keyword, resulting in the label being lowercase
regardless of the user input.

Signed-off-by: Panagiotis Foliadis<pfoliadis@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrgemignani jrgemignani jrgemignani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reserved keywords, when used as label in MATCH clause, is converted to lowercase
3 participants
@panosfol @rafsun42 @jrgemignani