Skip to content

[CALCITE-7603] Support ROW constructors that name fields#5018

Open
mihaibudiu wants to merge 2 commits into
apache:mainfrom
mihaibudiu:issue7603
Open

[CALCITE-7603] Support ROW constructors that name fields#5018
mihaibudiu wants to merge 2 commits into
apache:mainfrom
mihaibudiu:issue7603

Conversation

@mihaibudiu

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7603

Changes Proposed

This introduces the following syntax for a ROW values constructor: ROW(emp AS emp, deptno * 20 AS dept). The type of this expression is ROW(emp INT, dept INT).

The implementation is a bit unusual: instead of using AS operators the SqlRowOperator carries the field names if they are present. (If not, the behavior is completely unchanged). So there is no longer a singleton SqlRowOperator used. The names only need to be carried up to type inference, where they are used to infer the result ROW type. So the Rel part of the world knows nothing about this change.

@xiedeyantu xiedeyantu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test cases look good. I'm not very familiar with the parser section, so if you don't see any issues, feel free to go ahead.

* Support ROW constructors that name fields</a>. */
@Test void testRowWithFieldNames() {
// All fields named: the returned type should use the specified names
sql("select row(1 as a, 'hello' as b) from emp")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add a case like this?

select row(1 a, 'hello' b) from emp

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This case will fail, the AS is not optional in the ROW constructor.
Would you like it to be?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a new commit, since I moved some tests to a better place.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This case will fail, the AS is not optional in the ROW constructor. Would you like it to be?

I don't really have a strong preference either way, as long as the features are clearly defined. I also noticed you added the requirement to use AS, so I'm all good now.

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
…ative test

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants