Skip to content

Added support for json functions#546

Merged
Y-- merged 28 commits intoduckdb:mainfrom
ritwizsinha:json-functions
Feb 3, 2025
Merged

Added support for json functions#546
Y-- merged 28 commits intoduckdb:mainfrom
ritwizsinha:json-functions

Conversation

@ritwizsinha
Copy link
Contributor

@ritwizsinha ritwizsinha commented Jan 17, 2025

Addresses #516

@ritwizsinha
Copy link
Contributor Author

ritwizsinha commented Jan 17, 2025

@JelteF I have added all the json functions present in duckdb to the sql file, is this fine or do we need to break this up. All of this is quite repeated and most of it is tests, so I doubt a big PR is a problem

COMMENT ON DOMAIN pg_catalog.blob IS 'The DuckDB BLOB alias for BYTEA';

-- json_exists
CREATE FUNCTION @extschema@.json_exists(json VARCHAR , path VARCHAR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems confusing that these functions expect a VARCHAR type for the json argument. I'd expect that argument to be of the json or jsonb (or have functions for both of them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added for json

"iceberg_snapshots", "delta_scan", "read_json", "approx_count_distinct"};
const char *function_names[] = {"read_parquet", "read_csv", "iceberg_scan", "iceberg_metadata",
"iceberg_snapshots", "delta_scan", "read_json", "approx_count_distinct",
"json_exists", "json_extract", "json_extract_string"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems this list is missing a bunch of the functions that you added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@@ -0,0 +1,144 @@
-- <JSON_EXISTS>
-- Test 1: Path exists in a simple JSON object
SELECT json_exists('{"a": {"b": 1}}', '$.a.b'); -- Expected: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think most of these tests simply test that DuckDB works correctly, not so much the pg_duckdb intgration.

I think for each function it should give us enough confidence into pg_duckdb to add one test with reasonable arguments, and then tests with the various combinations of NULL arguments. So while I really appreciate the effort to add extensive tests, I don't think they add much value and would in practice only increase test runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced the tests

@ritwizsinha
Copy link
Contributor Author

@JelteF structures are not supported as types in pg_duckdb so not possible to implement functions like:

json_transform(json, structure)	Transform json according to the specified structure.
from_json(json, structure)	Alias for json_transform.
json_transform_strict(json, structure)	Same as json_transform, but throws an error when type casting fails.
from_json_strict(json, structure)	Alias for json_transform_strict.

Added all the other ones

@ritwizsinha ritwizsinha requested a review from JelteF January 22, 2025 14:14
@ritwizsinha
Copy link
Contributor Author

Curious why this fails on postgres 17

Copy link
Collaborator

@JelteF JelteF Jan 24, 2025

Choose a reason for hiding this comment

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

I just merged #531. Which introduces a more re-usable way of doing this:

AS 'MODULE_PATHNAME', 'duckdb_only_function'
LANGUAGE C;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@JelteF JelteF added the JSON Something related to JSON support label Jan 28, 2025
Copy link
Collaborator

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

I played around with this a bunch and added a few more tests and fixed a bunch of issues I found. I now think this is good enough to merge. @Y-- @mkaruza can either of you do another small review.

@wuputah wuputah requested review from Y-- and mkaruza February 3, 2025 20:09
@Y-- Y-- merged commit d9bb1a3 into duckdb:main Feb 3, 2025
5 checks passed
@JelteF JelteF mentioned this pull request Mar 18, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JSON Something related to JSON support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants