calcite [CALCITE-6365] Support for RETURNING clause of JSON

Add support for RETURNING clause of JSON_QUERY.

Apr 24 '24 12:04 dawidwys

There are only validator tests, but no tests that show such expressions evaluated. Is this planned for a future issue?

Apr 24 '24 16:04 mihaibudiu

@mihaibudiu I extended the PR with runtime evaluation

Apr 25 '24 12:04 dawidwys

When you respond to comments you should not force-push, but rather add new commits, to make it easier to review what's new.

Apr 25 '24 18:04 mihaibudiu

When you respond to comments you should not force-push, but rather add new commits, to make it easier to review what's new.

Sorry about that. I usually do that and I did it for this PR. However a linter rule was failing for the commit message I put for the extra commit. This made me think Calcite community expects the commits to always be in a mergeable state. That's why I squashed them together.

Apr 26 '24 11:04 dawidwys

You can test your commits before submitting with ./gradlew build. In this case you can find some style problems with lines that are too long. What does this do when you try to return a value which has an incorrect type, e.g., you return an integer, but the value is an array? That is the kind of negative test I have in mind.

Apr 26 '24 16:04 mihaibudiu

What does this do when you try to return a value which has an incorrect type, e.g., you return an integer, but the value is an array? That is the kind of negative test I have in mind.

I see. It makes it much easier to address your comments with a clear explanation. Thanks. It should behave the same way as JSON_VALUE does. Could you point me to tests that check that behaviour of JSON_VALUE? I'll adapt those. If you can't I'll try to write some myself once I am back from my vacation, but I'd really appreciate pointers to existing tests.

You can test your commits before submitting with ./gradlew build

Sorry about that, ./gradlew build fails even on clean master locally for me so it is hard for me to rely on it, but you're right it would've caught the line that was too long. I fixed it.

FAILURE   6.6sec,   22 completed,   1 failed,   3 skipped, org.apache.calcite.adapter.os.OsAdapterTest
          7.0sec, org.apache.calcite.chinook.EndToEndTest > test(String)[1], [1] sql/preferred-for-specific-user.iq
          8.8sec,    4 completed,   0 failed,   0 skipped, org.apache.calcite.chinook.EndToEndTest > test(String)
          8.8sec,    4 completed,   0 failed,   0 skipped, org.apache.calcite.chinook.EndToEndTest

Btw, as a side not. I've not written it before, because I found it unrelated to this PR, but I find this issue: https://issues.apache.org/jira/browse/CALCITE-6208 a bit useless. You can't really return an ARRAY from a JSON_VALUE, because it can only produce scalar values (which is in line with SQL standard). See JsonFunctions#jsonValue and e.g. https://github.com/apache/calcite/blob/5e837d02d0ba9e0b1e548acb5c218616234936c8/core/src/main/java/org/apache/calcite/runtime/JsonFunctions.java#L292 or https://github.com/apache/calcite/blob/5e837d02d0ba9e0b1e548acb5c218616234936c8/core/src/main/java/org/apache/calcite/runtime/JsonFunctions.java#L278

Apr 26 '24 17:04 dawidwys

I am relatively new to Calcite myself, and I wasn't a contributor when JSON_QUERY was introduced. So I search for tests probably in the same way you do, using file search.

I am not asking to test exhaustively JSON_QUERY in this PR, that would be a daunting effort, and maybe there aren't enough tests in Calcite. I just want to make sure that there are tests for the new feature that is implemented. I have noticed that many times the negative tests uncover bugs, so I think it's very useful to have a few of them as well.

If gradlew fails on a clean checkout maybe you should file an issue or quiz the dev mailing list.

I think you should add the comments above about issue CALCITE-6208 to the issue itself in JIRA. If that helps close the issue, great. I think the community prefers to have design decisions in the JIRA issues. But note that Calcite supports many SQL dialects, and sometimes offers additional functionality if it is reasonable.

Apr 26 '24 18:04 mihaibudiu

I was not verbose enough on the JIRA description. I am interested in supporting only part of the entire standard : RETURNING <data_type> without the FORMAT ... part (similarly to what is already done for JSON_VALUE.

Would you be ok with splitting the task and limiting this PR to only RETURNING <data_type>?

I updated the JIRA description

May 08 '24 13:05 dawidwys

Thanks for clarification