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

4.2.0 regression : basic query using 'in' keyword working 4.1.11 and returning no result in 4.2.0 #1829

Closed
jbrazeau opened this issue Nov 10, 2023 · 6 comments

Comments

@jbrazeau
Copy link

jbrazeau commented Nov 10, 2023

I've upgraded alasql (4.1.10 -> 4.2.0) this morning and faced a regression.

I've isolated the problem in a trivial test available in the following git repository : https://gitlab.com/jean-francois.brazeau/alasql-bug-4.2.0

The repository contains a very basic test that :

  • creates a simple table (2 columns)
  • insert two rows
  • selects the rows using in keyword

Simply run :

  • go-4.1.11.sh to install [email protected] and successfully run the test.
  • go-4.2.0.sh to install [email protected] and notice that the query returns no result at the end of the script.
@paulrutter
Copy link
Contributor

paulrutter commented Nov 10, 2023

Performance improvements yesterday were indeed affecting IN and NOT IN, so it seems something was missed in the unit test coverage.
I did add a similar test though, so not sure what causes it yet. I might investigate next week.

Probably related to using ? as using direct data assignments worked in the unit test.

@paulrutter
Copy link
Contributor

@jbrazeau i have been able to reproduce and confirmed that my PR is indeed causing the regression.
I will let you know once i have a fix.

paulrutter added a commit to blueconic/alasql that referenced this issue Nov 10, 2023
- Fix regression when using IN / NOT IN
- Using refs didn't work anymore
- Dropped set cache, as it's not really needed unless the same queries are performed often
- Added unit test to guard behavior
@paulrutter
Copy link
Contributor

@jbrazeau see related PR which should fix it.

@jbrazeau
Copy link
Author

jbrazeau commented Nov 10, 2023

Thank you very much for paying attention to that issue.

I've checked out the PR, built alasql and manually "patched" dist/alasql.fs.js in my node_modules folder (after having installed [email protected]). I confirm that the regression seems to disappears ! 👍

@paulrutter
Copy link
Contributor

I would propose merging this and release as 4.2.1.
We can check to re-add the cache layer for the Set later, now we have added more unit test coverage.

@mathiasrw
Copy link
Member

Released as part of 4.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants