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

Fix operation type detection #139

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Qup42
Copy link
Member

@Qup42 Qup42 commented Feb 24, 2025

Resolves #137, Resolves #138, Resolves ad-freiburg/qlever#1838

@hannahbast
Copy link
Member

@Qup42 Thanks a lot for the quick fix. It does fix the queries from #137 and #138, but this query still fails: https://qlever.cs.uni-freiburg.de/osm-planet/pgD6OH

@Qup42
Copy link
Member Author

Qup42 commented Feb 25, 2025

@hannahbast I have adjusted the regexes for the query. As we see by there being so many Issues with identifying whether an operation is a Query or an Update, using Regexes for that is a flawed approach. It's tricky to get right. I have opened IoannisNezis/Qlue-ls#22 to move the logic of identifying there. This is a much better place, because it uses the parsed query for identifying the type.

@hannahbast
Copy link
Member

@Qup42 Thanks a lot for the additional fix. I agree that a full parse would be better if we had it. But until we have it, is there something wrong (or complexity that I am missing) with the following approach:

  1. Remove all prefix declarations from the query
  2. Remove all comments from the query
  3. Remove leading whitespace from the query
  4. Look at the first keyword

In my understanding, each of these can be done correctly with a REGEX. Comment removal is tricky after the first keyword because the comment symbol can also occur in literals and IRIs. But for the purpose of query type detection, we do not care about anything after the first keyword.

@Qup42
Copy link
Member Author

Qup42 commented Feb 25, 2025

@hannahbast that's basically what is happening at the moment, with the difference that the two first keywords are looked at to determine the exact type. This could be removed, but it also isn't the part that caused the problems. The problems were caused by incorrect regexes and missing the regex to remove comments. One thing that has to though of is that the .* in the regex must not be greedy, else too much might be matched (like in your query). This and other things make it somewhat tricky.

@hannahbast
Copy link
Member

@Qup42 Tricky I understand. But do I understand you right that you are confirming that with the right regexes, this can be done correctly?

(That is a non-trivial, but I think true, statement because general SPARQL parsing cannot be done correctly with a few regexes of reasonable complexity.)

@Qup42
Copy link
Member Author

Qup42 commented Feb 25, 2025

@hannahbast We have no nesting and only some simple items. I think that that should be possible, yes.

@Qup42 Qup42 marked this pull request as ready for review February 25, 2025 17:28
@Qup42
Copy link
Member Author

Qup42 commented Feb 25, 2025

@hannahbast now we're doing it the right way by using qlue-ls. This is a good intermediate solution until the editor is replaced fully. It uses the same parsing as the formatter and then looks at the type of the corresponding clause to determine the type. Thanks a lot to @IoannisNezis for helping with the required changes and enabling an extremely quick turnaround. Great service, would use it again ;) 5/5

@hannahbast
Copy link
Member

@Qup42 @IoannisNezis I tried to build a docker image with this change and got:

=> [5/8] RUN set -ex     && runDeps="$(scanelf --needed --nobanner --recursive /env     | awk '{ gsub(/,/, "nso:", $2); print "so:" $2 }'     | sort -u     | xarg  2.4s 
 => ERROR [6/8] RUN npm ci                                                                                                                                           1.8s 
------                                                                                                                                                                    
 > [6/8] RUN npm ci:                                                                                                                                                      
1.721 npm error code ETARGET                                                                                                                                              
1.721 npm error notarget No matching version found for qlue-ls-web@~0.4.0.                                                                                                
1.722 npm error notarget In most cases you or one of your dependencies are requesting                                                                                     
1.722 npm error notarget a package version that doesn't exist.
1.725 npm error A complete log of this run can be found in: /root/.npm/_logs/2025-02-25T23_14_24_816Z-debug-0.log
------
Dockerfile:23
--------------------
  21 |     
  22 |     # install js deps
  23 | >>> RUN npm ci
  24 |     RUN npm run build
  25 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c npm ci" did not complete successfully: exit code: 1

@IoannisNezis
Copy link
Collaborator

IoannisNezis commented Feb 25, 2025

@hannahbast my bad, should work now.

@IoannisNezis IoannisNezis self-requested a review February 26, 2025 00:05
@IoannisNezis
Copy link
Collaborator

IoannisNezis commented Feb 26, 2025

hm, the package-lock.json is also not up to date.

I would fix this, but i seem not to have the permissions...

To fix just run:

npm install

and push the new package-lock.json
This updates the lock file.

@IoannisNezis IoannisNezis removed their request for review February 26, 2025 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants