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.

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

GH-2864: Fix algebra function name for the hash functions. #2865

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

afs
Copy link
Member

@afs afs commented Nov 23, 2024

GitHub issue resolved #2864

The algebra operator symbol should be lower case.
Print in SPARQL is overridden to be upper case as it has been for a long time.

Some reformatting in-passing.


  • Tests are included
  • Key commit messages start with the issue number (GH-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.

@afs afs changed the title GH-2864: GH-2864: Fix algebra function name for the hash functions. Nov 23, 2024
Copy link
Member

@rvesse rvesse left a comment

Choose a reason for hiding this comment

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

Change looks good

Do we want some test cases that cover the scenario that prompted the bug report?

Given your analysis that this has existed basically forever in the code base that strongly implies there are no pre-existing test cases that expose this bug

@afs
Copy link
Member Author

afs commented Nov 25, 2024

It's a display bug in printing algebra. The execution of queries isn't affected which presumably is why is hasn't come to light.

With the exception of qparse, I can't think of any non-development case of writing algebra derived from query syntax in a way that is going to be read in again. Writing is mainly for development, and parsing algebra for testing isn't based on the output of query syntax. Some of this "grew" in stages over an extended (pre-ASF) period of time.

@afs afs force-pushed the gh2864-hash-name branch from 610a8eb to 1a65ba2 Compare November 27, 2024 16:18
@afs afs requested a review from rvesse November 27, 2024 16:58
@rvesse rvesse merged commit c720c3f into apache:main Dec 4, 2024
4 checks passed
@afs
Copy link
Member Author

afs commented Dec 4, 2024

@rvesse - please use "rebase and merge".

The choice should be sticky by repo and not affect your choice elsewhere.

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.

is this valid sparql or does qparse have a bug?
2 participants