-
Notifications
You must be signed in to change notification settings - Fork 181
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
feat(sql): Adds JsonScanBuilder to daft-scan and read_json to daft-sql #3683
Conversation
CodSpeed Performance ReportMerging #3683 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3683 +/- ##
==========================================
- Coverage 77.82% 77.63% -0.19%
==========================================
Files 728 729 +1
Lines 89919 90290 +371
==========================================
+ Hits 69975 70098 +123
- Misses 19944 20192 +248
|
let glob_paths: String = args | ||
.try_get_positional(0)? | ||
.ok_or_else(|| PlannerError::invalid_operation("path is required for `read_json`"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary in this PR, but I think it'd be nice to also support an array of paths here (we support this in the dataframe API)
something like
select * from read_json(['./file1.json', './file2.json'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an issue for all three to support the array of paths.
Description
This PR follows the read_csv and read_parquet pattern for adding the table-value function to SQL. This issue solves/addresses #3196 but there a couple TODOs which could be tackled in later PRs.
Issues
read_json
function #3196