Fixing 2439: Improve Copy Option Usability #4230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_2439
branch
from
d93d187 to
fb8cac7
Compare
SterlingT3485
force-pushed
the
sterling_2439
branch
from
ef4ec49 to
e6fb471
Compare
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_2439
branch
from
ff60c60 to
e1f95ce
Compare
SterlingT3485
force-pushed
the
sterling_2439
branch
from
038b5b8 to
a6be115
Compare
Benchmark ResultMaster commit hash:
|
ray6080
reviewed
Sep 11, 2024
There was a problem hiding this comment.
Generally looks good to me. Thanks!
Can u add some copy test cases for these changes? Also, please update our related docs on copy options and open an PR against the dev branch here.
Will take another quick look after the changes.
SterlingT3485
force-pushed
the
sterling_2439
branch
from
bb47c53 to
6734273
Compare
1 task
|
The corresponding DOCS update PR: kuzudb/kuzu-docs#265 |
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
SterlingT3485
force-pushed
the
sterling_2439
branch
2 times, most recently
from
8aab3e0 to
b7b34b5
Compare
Benchmark ResultMaster commit hash:
|
ray6080
reviewed
Sep 16, 2024
There was a problem hiding this comment.
I don't see a test on DELIMITER. Can u add a test on that too?
There was a problem hiding this comment.
sure, add it now
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4230 +/- ##
==========================================
- Coverage 87.31% 86.74% -0.58%
==========================================
Files 1327 1327
Lines 51271 51593 +322
Branches 6884 6951 +67
==========================================
- Hits 44769 44756 -13
- Misses 6330 6665 +335
Partials 172 172 ☔ View full report in Codecov by Sentry. |
Benchmark ResultMaster commit hash:
|
ted-wq-x
pushed a commit
to ted-wq-x/kuzu
that referenced
this pull request
Nov 14, 2024
* enable 3 types of alternative boolean expression * make the grammar accept both SP and = for equation * replacing hash.md5 * enable default boolean value * change name to be reference * support both DELIM and DELIMITER as option name * Run clang-format * add DELIMITER * add test for boolean options * force to compile the grammar * Run clang-format * add compiled parser * skip grammar update * add omit_default and DELIMITER test --------- Co-authored-by: sterling <[email protected]> Co-authored-by: CI Bot <[email protected]> (cherry picked from commit 7c69437)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes 2439
This Makes our Copy Option to be more usable, now the options are compatible with the DuckDB syntax and simultaneously keep our original syntax.
We support multiple ways of expressing Boolean value (
0/1; on/off; true/false), and also support both empty space and equation sign between the name of option and the value of option.We also support omitted boolean option value, which would be set to
trueas default.Contributor agreement