7) Fix getTables request #75
Conversation
pivnicek
force-pushed
the
piv-fix-gettables
branch
from
a7cd16f
to
8bd3033
Compare
Halama
force-pushed
the
piv-fix-gettables
branch
from
8bd3033
to
b624fa3
Compare
|
I've rebased it |
| $tableDefs = []; | ||
| foreach ($arr as $table) { | ||
| if ($this->shouldTableBeSkipped($table)) { | ||
| continue; | ||
| } | ||
| if (is_null($tables) || !(array_search($table['name'], array_column($tables, 'tableName')) === false)) { | ||
| $tableNameArray[] = $table['name']; |
There was a problem hiding this comment.
I am still a little bit lost in a logic of this function. I think it would be more clear to:
-
- fetch array of all tables with it schema associated which should be fetched. It is
$tableDefsarray now.
- fetch array of all tables with it schema associated which should be fetched. It is
-
- fetch columns for all tables from
$tableDefs. So the SQL would look like:
- fetch columns for all tables from
SELECT * FROM information_schema.columns WHERE
(table_schema = 'snowflake_extractor' AND table_name = 'LARGE')
OR
(table_schema = 'snowflake_extractor' AND table_name = 'LARGE_100M')
;
So than you can be sure that really only columns for requested tables are fetched and there is less conditions.
|
@Halama , sorry I let this slip, I've updated it and think it's now more clear. |
|
@Halama az budes mit kvilicku muzes na tohle dat oko? rad bych ho releasnout. |
|
@pivnicek I am not sure when I'll get to this:( Maybe try someone other for review? |
|
@Halama sure, no worries |
| @@ -17,7 +17,7 @@ | |||
| "keboola/php-csv-db-import": "~2.3.0" | |||
| }, | |||
| "require-dev": { | |||
| "phpunit/phpunit": "4.6.*", | |||
| "phpunit/phpunit": "^7.0", | |||
There was a problem hiding this comment.
Why the upgrade? And why not upgrade anything else (cs, phpstan)? I have to say I much prefer to create separate PR for any non-related changes.
There was a problem hiding this comment.
There must have been a reason, I think to match with the common-lib version, but you're right, should have been better done somewhere else. Shall I move it, or can we keep it just this once ;-)
|
Thanks @tomasfejfar ! I've updated it. |
|
ok, I'm going to merge this then 🎉 |
Fixes #72
Rebased onto #71