[CALCITE-6115] Interval type specifier with zero fractional second precision does not pass validation

[LeonidChistov]

No description provided.

[mihaibudiu]

it is customary to add a comment with the JIRA issue that is being solved.
Please follow the other examples in this file that start with "Test case for".
Note that everything is important, including the last period.

[LeonidChistov]

Thanks, fixed.

[mihaibudiu]

I am not a SQL expert, but if other dialects accept this, I think it's good.

[LeonidChistov]

@mihaibudiu

What exactly do you mean regarding SQL dialects support? Since Calcite is a framework for building RDBMS query optimizers, I think it make sense for it to support this feature described in SQL standard, even if some databases would not support it.
It is always possible to add custom checks that would prohibit zero fractional precision, but if it is prohibited on framework level, there is no easy way to disable these checks if you try to build SQL-standard compliant RDBMS.

[sonarcloud]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
80.0% Duplication on New Code

See analysis details on SonarCloud

[LeonidChistov]

@mihaibudiu

Thanks a lot for review. BTW, do you have a power to trigger Slow Tests run? Looks like it doesn't run by default.

[mihaibudiu]

I am not aware of the Calcite CI running slow tests. I don't know how to do it.

[libenchao]

@LeonidChistov @mihaibudiu The slow tests are trigger by adding a label "slow-tests-needed", see here:

calcite/.github/workflows/main.yml

Lines 401 to 404 in 8c640da

	linux-slow:
	# Run slow tests when the commit is on main or it is requested explicitly by adding an
	# appropriate label in the PR
	if: github.ref == 'refs/heads/main' || contains(github.event.pull_request.labels.*.name, 'slow-tests-needed')

[LeonidChistov]

@libenchao Thanks. Do I understand correctly that this test run is not required (in general) to merge a PR?

[asolimando]

@libenchao Thanks. Do I understand correctly that this test run is not required (in general) to merge a PR?

They are not, but they will be run when the PR is merged, for changes to core functions that can have a broad impact I'd say it's recommended, in order to catch issues early on without waiting for the main branch to break.

What is a "core function" is a fuzzy definition though, you can look for the @Tag("slow") tag to get a sense of that is covered by slow tests (even though, the scope is pretty broad).

If anyone has some more precise recommendations on this topic, I guess it would be a great addition to the developer's guide on the website.

[LeonidChistov]

@asolimando thanks a lot for explanation.

Could someone help now with merging the PR?