[native] Fix varchar cast for json by natashasehgal · Pull Request #24396 · prestodb/presto

natashasehgal · 2025-01-17T23:06:25Z

Description

Resolve error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)".

Motivation and Context

This is not due to missing function in Prestissimo, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length.
Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR

Impact

N/A

Test Plan

Added unit test

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

facebook-github-bot · 2025-01-17T23:06:35Z

This pull request was exported from Phabricator. Differential Revision: D68353517

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-21T22:49:56Z

This pull request was exported from Phabricator. Differential Revision: D68353517

amitkdutta · 2025-01-22T21:30:01Z

@natashasehgal Thanks for the fix. Please publish the PR, its in draft mode. Also make the PR title and commit message following
"[native] Fix varchar cast for json".

Lets also follow the PR template and fill in accordingly.

amitkdutta

Looks good. Lets update the commit message also.

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-22T23:13:06Z

This pull request was exported from Phabricator. Differential Revision: D68353517

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-22T23:13:45Z

This pull request was exported from Phabricator. Differential Revision: D68353517

amitkdutta

Looks good. Thanks @natashasehgal

gggrace14

Request double check

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-24T02:00:34Z

This pull request was exported from Phabricator. Differential Revision: D68353517

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-24T02:05:11Z

This pull request was exported from Phabricator. Differential Revision: D68353517

kgpai

Looks good. Can you add an e2e test ?

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-24T21:46:54Z

This pull request was exported from Phabricator. Differential Revision: D68353517

facebook-github-bot · 2025-01-24T21:47:04Z

This pull request was exported from Phabricator. Differential Revision: D68353517

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-24T21:47:30Z

This pull request was exported from Phabricator. Differential Revision: D68353517

natashasehgal · 2025-01-24T22:11:34Z

Looks good. Can you add an e2e test ?

Yes. @kgpai could you please point me to a suitable location to add an e2e test? I wasn't able to find an e.g for presto-native-execution changes

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

facebook-github-bot · 2025-01-27T20:06:22Z

This pull request was exported from Phabricator. Differential Revision: D68353517

kgpai

Thanks, LGTM.

gggrace14

Thank you @natashasehgal for iterating on this change and studying the behavior thoroughly! The impl looks right to me.

gggrace14 · 2025-01-30T21:26:39Z

        // Round-trip tests of casts for Json.
        assertQuery("SELECT cast(cast(name as JSON) as VARCHAR), cast(cast(size as JSON) as INTEGER), cast(cast(size + 0.01 as JSON) as DOUBLE), cast(cast(size > 5 as JSON) as BOOLEAN) FROM part");
        assertQuery("SELECT cast(cast(array[suppkey, nationkey] as JSON) as ARRAY(INTEGER)), cast(cast(map(array[name, address, phone], array[1.1, 2.2, 3.3]) as JSON) as MAP(VARCHAR(40), DOUBLE)), cast(cast(map(array[name], array[phone]) as JSON) as MAP(VARCHAR(25), JSON)), cast(cast(array[array[suppkey], array[nationkey]] as JSON) as ARRAY(JSON)) from supplier");
+        assertQuery("SELECT cast(json_extract(x, '$.a') AS varchar(255)) AS extracted_value FROM (VALUES ('{\"a\": \"Some long string\"}')) AS t(x)");


Would be better to add one more test case of assertQueryFails(). But I don't want to block the merging of the PR.

Great idea. I merged this already, will follow up with an appropriate failure test case

Summary: https://www.internalfb.com/tasks/?t=211442303 There was an error in running query on Prestissimo not Presto - "Scalar function presto.default.substr not registered with arguments: (JSON, BIGINT, BIGINT)". This is not due to missing function, as the function signature does not exist in Presto. It occurs when attempting to cast JSON as varchar of capped length. Related Diff: https://www.internalfb.com/diff/D59531026 Note: Exception is still raised for try_cast() behavior. Alignment is out of scope for this PR Differential Revision: D68353517

natashasehgal requested a review from a team as a code owner January 17, 2025 23:06

prestodb-ci added the from:Meta PR from Meta label Jan 17, 2025

facebook-github-bot added the fb-exported label Jan 17, 2025

natashasehgal marked this pull request as draft January 17, 2025 23:09

natashasehgal force-pushed the export-D68353517 branch from f6c1132 to 98a84af Compare January 21, 2025 22:49

natashasehgal marked this pull request as ready for review January 22, 2025 22:06

natashasehgal changed the title ~~Fix varchar cast for json~~ [native] Fix varchar cast for json Jan 22, 2025

amitkdutta reviewed Jan 22, 2025

View reviewed changes

Comment thread presto-native-execution/presto_cpp/main/types/tests/RowExpressionTest.cpp

natashasehgal force-pushed the export-D68353517 branch from 98a84af to 5309841 Compare January 22, 2025 23:12

natashasehgal force-pushed the export-D68353517 branch from 5309841 to 84144d1 Compare January 22, 2025 23:13

natashasehgal requested a review from amitkdutta January 22, 2025 23:14

amitkdutta previously approved these changes Jan 22, 2025

View reviewed changes

kgpai previously approved these changes Jan 22, 2025

View reviewed changes

gggrace14 requested changes Jan 23, 2025

View reviewed changes

Comment thread presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp Outdated

Comment thread presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp

gggrace14 self-requested a review January 23, 2025 06:27

gggrace14 requested changes Jan 23, 2025

View reviewed changes

gggrace14 self-requested a review January 23, 2025 06:32

natashasehgal dismissed stale reviews from kgpai and amitkdutta via 997643c January 24, 2025 02:00

natashasehgal force-pushed the export-D68353517 branch from 84144d1 to 997643c Compare January 24, 2025 02:00

natashasehgal force-pushed the export-D68353517 branch from 997643c to 1a3f520 Compare January 24, 2025 02:04

natashasehgal requested a review from kgpai January 24, 2025 02:07

kgpai reviewed Jan 24, 2025

View reviewed changes

natashasehgal force-pushed the export-D68353517 branch from 1a3f520 to 5cafb02 Compare January 24, 2025 21:46

natashasehgal force-pushed the export-D68353517 branch from 5cafb02 to 35b8915 Compare January 24, 2025 21:47

natashasehgal force-pushed the export-D68353517 branch from 35b8915 to 3449a8c Compare January 27, 2025 20:05

natashasehgal requested a review from kgpai January 29, 2025 21:41

kgpai approved these changes Jan 30, 2025

View reviewed changes

gggrace14 approved these changes Jan 30, 2025

View reviewed changes

natashasehgal merged commit 719bfe2 into prestodb:master Jan 30, 2025

natashasehgal deleted the export-D68353517 branch January 30, 2025 21:34

This was referenced Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

Add release notes for 0.292 unix280/presto#6

Closed

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

Conversation

natashasehgal commented Jan 17, 2025 • edited by amitkdutta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

facebook-github-bot commented Jan 17, 2025

Uh oh!

facebook-github-bot commented Jan 21, 2025

Uh oh!

amitkdutta commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitkdutta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented Jan 22, 2025

Uh oh!

facebook-github-bot commented Jan 22, 2025

Uh oh!

amitkdutta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gggrace14 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 24, 2025

Uh oh!

facebook-github-bot commented Jan 24, 2025

Uh oh!

kgpai left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 24, 2025

Uh oh!

facebook-github-bot commented Jan 24, 2025

Uh oh!

facebook-github-bot commented Jan 24, 2025

Uh oh!

natashasehgal commented Jan 24, 2025

Uh oh!

facebook-github-bot commented Jan 27, 2025

Uh oh!

kgpai left a comment

Choose a reason for hiding this comment

Uh oh!

gggrace14 left a comment

Choose a reason for hiding this comment

Uh oh!

gggrace14 Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

natashasehgal Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

natashasehgal commented Jan 17, 2025 •

edited by amitkdutta

Loading

amitkdutta commented Jan 22, 2025 •

edited

Loading