CT-1424 remove dependency on native types feature #175
Conversation
… so it would always return true
| @@ -498,12 +498,12 @@ public function getUpdateWithPkCommandProvider(): Generator | |||
| yield 'typed' => [ // nothing is converted | |||
| BigqueryImportOptions::USING_TYPES_USER, | |||
| // phpcs:ignore | |||
| 'UPDATE `import_export_test_schema`.`import_export_test_test` AS `dest` SET `col1` = `src`.`col1`, `col2` = `src`.`col2` FROM `import_export_test_schema`.`stagingTable` AS `src` WHERE `dest`.`col1` = `src`.`col1` ' | |||
| 'UPDATE `import_export_test_schema`.`import_export_test_test` AS `dest` SET `col1` = `src`.`col1`, `col2` = `src`.`col2` FROM `import_export_test_schema`.`stagingTable` AS `src` WHERE `dest`.`col1` = `src`.`col1` AND (`dest`.`col1` IS DISTINCT FROM `src`.`col1` OR `dest`.`col2` IS DISTINCT FROM `src`.`col2`)' | |||
There was a problem hiding this comment.
tady mi dava smysl mit ten IS DISTINCT FROM, ale i te stringove tabulky? nemel by tam byt spis ``col2 = COALESCE(src`.`col2`, '')` ? Ale zas na druhou stranu proc, ten IS DISTINCT FROM by mel zvladat i nully?
There was a problem hiding this comment.
Už si začínám vzpomínat proč sa chcu zabit pokaždé když vlezu do importů :D Docela sa divím proč to tak je a proč sa tam hodnoty neporovnávajů, určitě to má nějaký debilní, teda dobrý důvod, který si nepamatuju.
Ve string tabulkách null može byt na straně workspace když tam má uživatel tabulku s typama a dělá import do netypové ve storage, pak to udělá takto update protože null a '' je distinct.
Takže je to krapet lepší než to bylo, ale ten COALESCE by tam měl byt spíš. Pak sa coalesce(null,'') = '' a chová sa to stejně.
Ale za boha si nemožu vzpomnět proč sa tam ty hodnoty neporovnávajů.
There was a problem hiding this comment.
Zajímavé tady sem to přidal a od té doby sa na to nehráblo keboola/php-db-import-export#157
There was a problem hiding this comment.
imho je to tim, ze jsme byli ve vleku tyopvanych tabulek na SNFLK . Tam v tu chvili byla "dohoda" - porovnavame jen PK (protoze NULL problem) - tak jsme udelali takto i BQ implemebtaci
There was a problem hiding this comment.
kazdopadne ten coalesce tam pridam
| @@ -381,20 +313,13 @@ public function testImportTimestampBehavior(array $features, array $expectedCont | |||
| `name` STRING(50), | |||
| `price` DECIMAL, | |||
| `isDeleted` INT64, | |||
| `_timestamp` TIMESTAMP | |||
| `_timestamp` TIMESTAMP, | |||
| PRIMARY KEY(id) NOT ENFORCED | |||
There was a problem hiding this comment.
ten PK se z nejakeho duvodu nenastavoval, ale pred tim to asi nevadilo, bo porovnavani hodnot bylo rizene ficurou. ted uz neni a je ten PK tam realne potreba
There was a problem hiding this comment.
S feature to nemá nic společného o řádek níž sa přidával ten PK a tys to smazal :D Chvilku mě trvalo než sem na to došel.
There was a problem hiding this comment.
smazal, protoze to realne ten PK nepridavalo. nevim proc, ale proste do tableDefinition to doslo bez PK
There was a problem hiding this comment.
a proto jsem to pridal sem. S ficurou to melo podle me spolecne to, ze ta ficura ovlivnovala to, zda se PK realne pouzilo nebo ne. Ale na to se musim jeste podivat hloubeji
There was a problem hiding this comment.
Tak to nevím teď sem to testoval u sebe a když smažu ten PRIMARY KEY a přidám tam ten update co tam byl tak ten test mě prošel, nemáš třeba staré závislosti?
There was a problem hiding this comment.
nevylucuju, zkusim
There was a problem hiding this comment.
hele tak to asi bylo zavislostma. Upgrading google/cloud-bigquery (v1.24.0 => v1.31.0) a uz to frci, toto teda vratim
| $stagingTable = StageTableDefinitionFactory::createVarcharStagingTableDefinition( | ||
| $destination->getSchemaName(), | ||
| [ | ||
| $columnCollection = new ColumnCollection([ |
There was a problem hiding this comment.
connection uz vyrabi typovane stage tabulky, takze se musi i tady
There was a problem hiding this comment.
Jakože teoreticky to ta lib dovoluje pořád. Už bych ju radši zrušil :D
There was a problem hiding this comment.
jo, ale chceme to testovat? nebo asi jo, ale primarne chci videt case s tim co od te libky chceme
There was a problem hiding this comment.
jj asi není potřeba mět pokrytů každů kravinu
| TO_VARCHAR(TO_BINARY(HEX_ENCODE(\'1\'), \'HEX\')), | ||
| TO_VARCHAR(OBJECT_CONSTRUCT(\'name\', \'Jones\'::VARIANT, \'age\', 42::VARIANT)), | ||
| TO_VARCHAR(ARRAY_CONSTRUCT(1, 2, 3, NULL)), | ||
| TO_VARIANT(\'3.14\'), |
There was a problem hiding this comment.
stage tabulka je typovana
There was a problem hiding this comment.
Padá tam ještě jeden testík.
Otázka je co stým getUpdateWithPkCommand u BQ, tam bych logicky nechal, že sa to neporovnává, ale stejně tak měli všeci teď tu feature a porovnávalo sa to přes distinct :D Takže ten současný kód je vlastně to co je teď na produkci. 🤔
jirkasemmler
deleted the
jirka-ct-1424-remove-dependency-on-native-types-feature
branch
Jira: CT-1424
Connection PR: https://github.com/keboola/connection/pull/5347
SAPI PR: -
I have dropped the dependency on
native-types_timestamp-bcfeature because it should be by default these days-> I removed
compareAllColumnsInNativeTable-> SQL builder has been changed so now all INC load does comparing all the columns -> all the tests had to be fixed... kill me nowtag in connection was successful https://dev.azure.com/keboola-dev/Connection%20build/_build/results?buildId=51963&view=results