Use decimals by default in tpch connector#17344
Use decimals by default in tpch connector#17344raunaqmorarka wants to merge 2 commits intotrinodb:masterfrom
Conversation
raunaqmorarka
force-pushed
the
tpch-update
branch
3 times, most recently
from
c6d9422 to
d242a06
Compare
github-actions
bot
added
delta-lake
raunaqmorarka
force-pushed
the
tpch-update
branch
3 times, most recently
from
665826e to
5735f10
Compare
@findepi tests are green now, but it required a few more tweaks. Please take another look at it. |
There was a problem hiding this comment.
BTW if this is inserted in pretty much every query runner, how can i know that ATF would pass for a new test class that doesn't have this property?
There was a problem hiding this comment.
I had to do this because all these connectors have written test assertions based on double types in tpch. It was requiring a lot of trial and error to uncover and change every such instance. In some cases the test actually needs a double data type. In other cases decimal type is not supported by the connector.
You don't need to set this property for any new tests, it's there to avoid breaking tests which were written when tpch tables had double type.
There was a problem hiding this comment.
which BCT & BCST subclasses exercise BCT / BCST with tpch with decimals?
when will the existing tests migrated to use tpch with decimals?
There was a problem hiding this comment.
which BCT & BCST subclasses exercise BCT / BCST with tpch with decimals?
I've updated TestTpchConnectorTest to use decimals now. I'm guessing that you're concerned about whether some BCT is run with tpch+decimals rather than which connectors are specifically doing what.
when will the existing tests migrated to use tpch with decimals?
I think it is legitimate for other connectors and integration tests to configure tpch with doubles and continue to use that in their tests. There are tests which are specifically about double type and there are connectors which don't support decimals, so we definitely can't migrate everything. Whether we should migrate the tests which can be migrated depends on what's the benefit of making those changes.
There was a problem hiding this comment.
I've updated TestTpchConnectorTest to use decimals now.
tpch doesn't support create table, so i guess this test class has many tests skipped.
when will the existing tests migrated to use tpch with decimals?
I think it is legitimate for other connectors and integration tests to configure tpch with doubles and continue to use that in their tests.
absolutely.
but the end state should be consistent: all connectors that can use decimals, should use decimals
and exceptions to the rule should be have rationale documented
if we cannot achieve consistency that way, maybe we should revert 0c58983 instead
|
^ Just rebased to master |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
@raunaqmorarka is this still in progress? |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
3 participants
Description
Use decimals by default in tpch connector
Use simplified naming convention in tpch dev catalog
Additional context and related issues
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text: