Skip to content

test: check limit().evaluate() #315

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexander-mart
Copy link

@alexander-mart alexander-mart commented Apr 23, 2025

@hoijnet
Copy link
Collaborator

hoijnet commented Apr 24, 2025

I added an integration test for this case in this PR:
#316

It works as of now and I could not reproduce. The arithmetic happens on the server side so need to do the query. Please modify the integration tests to trigger the error as it would help fix the issue.

It seems there is some kind of corner case that triggers your issue, but I can't understand why or reproduce at the moment. With the integration test in place, it should be easier to boil it down hopefully.

The integration test for it is called "Test simple arithmetic with vars variable handling", there is an equivalent with the string variable handling.

Thanks for your efforts, let's squash it!

@hoijnet
Copy link
Collaborator

hoijnet commented Apr 24, 2025

The test you provided is not evaluated by the server and should thus look like this. Happy to pull in the test to ensure correct WOQL evaluation. This is the same WOQL as in the integration test.

  it('check limit().eval()', () => {
    let v = Vars("result");
    const woqlObject = WOQL.limit(100).eval(WOQL.times(2, 3), v.result);
    const expectedJson = {"@type":"Limit","limit":100,"query":{"@type":"Eval","expression":{"@type":"Times","left":{"@type":"ArithmeticValue","data":{"@type":"xsd:decimal","@value":2}},"right":{"@type":"ArithmeticValue","data":{"@type":"xsd:decimal","@value":3}}},"result":{"@type":"ArithmeticValue","variable":"result"}}};
    expect(woqlObject.json()).to.deep.eql(expectedJson);
  });

Let's try to find a reproducible case for how the incorrect WOQL with Value is generated.

@alexander-mart
Copy link
Author

alexander-mart commented Apr 24, 2025

@hoijnet I moved my answer to (original) issue, because there is full description a bug, but PR is only add test.

terminusdb/terminusdb#2155 (comment)

@hoijnet
Copy link
Collaborator

hoijnet commented Apr 24, 2025

@hoijnet I moved my answer to (original) issue, because there is full description a bug, but PR is only add test.

terminusdb/terminusdb#2155 (comment)

The issue is that the test is incorrect, as it is never executed; it needs to be queries from the server to get a correct response. Please look at the test added in #316 for reference. Does it make sense to you too?

@alexander-mart
Copy link
Author

@hoijnet What do you mean "it is never executed"? 🤔

The test is definitely executed and fails with an error that the corresponding evaluate() method on the limit object was not found (same error from dashboard):

TypeError: WOQL.limit(...).evaluate is not a function

Or I don't understand correctly


it needs to be queries from the server to get a correct response

How do all the other similar tests in this file work then?

@hoijnet
Copy link
Collaborator

hoijnet commented Apr 24, 2025

@hoijnet What do you mean "it is never executed"? 🤔

The test is definitely executed and fails with an error that the corresponding evaluate() method on the limit object was not found (same error from dashboard):

TypeError: WOQL.limit(...).evaluate is not a function

Or I don't understand correctly

it needs to be queries from the server to get a correct response

How do all the other similar tests in this file work then?

Realise I didn't write the full throught. In essence, two things here:

  • The evaluate() function seems to not work fully in chaining. eval() is a better option and more supported. Unsure why both are there, evaluate() should probably be cleaned up (or at least be hidden away from documentation). Please change to .eval() for the test
  • There are two steps in WOQL:
    1. the first is to transform the WOQL language to an Abstract Syntax Tree, what you get with the .json() function. That is what is checked against in the file, the "machine-readable WOQL code" that is sent to the server as a query for evaluation. Most of the tests ensure valid and correct WOQL is created.
    2. The second step is the query() to the server, where terminusdb runs the WOQL code, and the result is returned if the WOQL is correct. If the WOQL AST is incorrect (as with Value instead of ArithmeticValue for the variable), it will fail
  • We should test the eval() result for correctness in the client as a first step. The code in your original issue indicates that the client somehow incorrectly gets Value instead of ArithmeticValue in the WOQL machine-readable AST. Unsure why.

So what the .json() produces is only on the client side, it needs to be processed by terminusdb to yield the result you look for.

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.

2 participants