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

Replace better-eval with vm #536

Merged
merged 54 commits into from
Jun 28, 2023
Merged

Replace better-eval with vm #536

merged 54 commits into from
Jun 28, 2023

Conversation

sfc-gh-ext-simba-lf
Copy link
Collaborator

Regarding issue 366

The PR replaces better-eval with vm for converting raw variant strings. The better-eval package creates a new VM for each conversion while this change creates a single VM and re-uses it for all conversions

@sfc-gh-ext-simba-lf
Copy link
Collaborator Author

Added test case similar to the repro that calls select on a variant column containing a large result set

@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf marked this pull request as ready for review June 15, 2023 00:43
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf requested a review from a team as a code owner June 15, 2023 00:43
sfc-gh-igarish
sfc-gh-igarish approved these changes Jun 15, 2023
View reviewed changes
Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

Please do pref test and put numbers in this ticket before and after the change.

@sfc-gh-ext-simba-lf
Copy link
Collaborator Author

Please do pref test and put numbers in this ticket before and after the change.

Average execution time of testVariantLarge over 10 runs:

  • better-eval: 22.6s
  • vm: 4.4s

@sfc-gh-dszmolka sfc-gh-dszmolka mentioned this pull request Jun 15, 2023
Closed
sfc-gh-dprzybysz
sfc-gh-dprzybysz reviewed Jun 15, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
test/integration/testLargeResultSet.js Outdated Show resolved Hide resolved
test/integration/testLargeResultSet.js Outdated Show resolved Hide resolved
test/integration/testLargeResultSet.js Outdated Show resolved Hide resolved
test/integration/testLargeResultSet.js Outdated Show resolved Hide resolved
test/integration/testLargeResultSet.js Outdated Show resolved Hide resolved
test/integration/testLargeResultSet.js Outdated Show resolved Hide resolved
test/integration/testLargeResultSet.js Outdated Show resolved Hide resolved
@sfc-gh-ext-simba-lf
issue 366 - Remove 'vm' from package.json as it's already part of node
49be617
@sfc-gh-ext-simba-lf
issue 366 - Rename test
d6caa0a
@sfc-gh-ext-simba-lf
issue 366 - Make array const
48362b1
@sfc-gh-ext-simba-lf
issue 366 - Wrapped lines in else clause
73d1184
@sfc-gh-ext-simba-lf
issue 366 - Put variant test in its own describe and placed create/dr…
6e5ea30 
…op table in the before/after functions
@sfc-gh-ext-simba-lf
issue 366 - Put variant test in its own describe and placed create/dr…
a4116fe 
…op table in the before/after functions
@sfc-gh-ext-simba-lf
issue 366 - Put variant test in its own describe and placed create/dr…
6b48461 
…op table in the before/after functions
@sfc-gh-ext-simba-lf
issue 366 - Put variant test in its own describe and placed create/dr…
7648167 
…op table in the before/after functions
@sfc-gh-ext-simba-lf
issue 366 - Put variant test in its own describe and placed create/dr…
a386231 
…op table in the before/after functions
@sfc-gh-ext-simba-lf
issue 366 - Put variant test in its own describe and placed create/dr…
45d6ac7 
…op table in the before/after functions
@sfc-gh-ext-simba-lf
issue 366 - Temp revert wrapping test in describe
9909b2b
@sfc-gh-ext-simba-lf
issue 366 - Temp revert else clause
f17bd1d
@sfc-gh-ext-simba-lf
issue 366 - Temp revert const array
8cb7cbe
@sfc-gh-ext-simba-lf
issue 366 - Temp revert name change
7e490c8
@sfc-gh-ext-simba-lf
issue 366 - Temp revert remove vm
2fe6098
@sfc-gh-ext-simba-lf
issue 366 - Remove vm
ae4f82b
@sfc-gh-ext-simba-lf
issue 366 - Rename test
32828b1
sfc-gh-dprzybysz
sfc-gh-dprzybysz requested changes Jun 22, 2023
View reviewed changes
Copy link
Collaborator

@sfc-gh-dprzybysz sfc-gh-dprzybysz left a comment

Choose a reason for hiding this comment

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

Let's add run lint (npm run lint:check -- FILENAME) for new code. Especially new files could be even changed with npm run lint:fix -- FILENAME.

lib/core.js Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/global_config.js Show resolved Hide resolved
lib/global_config.js Outdated Show resolved Hide resolved
test/integration/testDataType.js Outdated Show resolved Hide resolved
test/unit/snowflake_config_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_config_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_config_test.js Outdated Show resolved Hide resolved
test/unit/snowflake_config_test.js Outdated Show resolved Hide resolved
@odedpeer
Copy link

Please do pref test and put numbers in this ticket before and after the change.

Average execution time of testVariantLarge over 10 runs:

  • better-eval: 22.6s
  • vm: 4.4s

what is the average execution time of eval? I'd like to see the difference between vm and eval

@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf merged commit 0926a0f into master Jun 28, 2023
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf deleted the blockingIssue branch June 28, 2023 17:34
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sfc-gh-pmotacki sfc-gh-pmotacki sfc-gh-pmotacki left review comments

@yanavais yanavais yanavais left review comments

@odedpeer odedpeer odedpeer left review comments

@sfc-gh-dprzybysz sfc-gh-dprzybysz sfc-gh-dprzybysz approved these changes

@sfc-gh-igarish sfc-gh-igarish Awaiting requested review from sfc-gh-igarish

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

Successfully merging this pull request may close these issues.
6 participants
@sfc-gh-ext-simba-lf @odedpeer @yanavais @sfc-gh-igarish @sfc-gh-dprzybysz @sfc-gh-pmotacki