Skip to content

[CALCITE-7531] Add to BasicSqlType constructor accepting precision, scale and nullability#4947

Open
snuyanzin wants to merge 1 commit into
apache:mainfrom
snuyanzin:calcite7531
Open

[CALCITE-7531] Add to BasicSqlType constructor accepting precision, scale and nullability#4947
snuyanzin wants to merge 1 commit into
apache:mainfrom
snuyanzin:calcite7531

Conversation

@snuyanzin
Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7531

Changes Proposed

before Calcite 1.38 it was possible to set nullability with setter
now it is impossible, also it is impossible to pass it via constructor together with precision and scale.

so Flink's code stops working after attempting to bump Calcite.
The PR adds constructor to resolve it

here it is Flink code using this https://github.com/apache/flink/blob/1990310135edfec7021f77e64dbb976100458909/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/schema/TimeIndicatorRelDataType.scala#L30-L38

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

I found it strange not having this API, especially since you actually need an instance of a typeSystem to change the nullability...

}

/**
* Constructs a type with precision/length and scale.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The javadoc on this constructor is identical to that on the previous constructor. And it doesn't describe what this constructor does.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worse than that, the change you have made isn't consistent with the commit message. Sort it out, please.

Copy link
Copy Markdown
Contributor

@julianhyde julianhyde left a comment

Choose a reason for hiding this comment

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

+1 when javadoc is fixed

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.

3 participants