Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batchinsert and associative arrays #61

Closed
SamMousa opened this issue Aug 10, 2018 · 12 comments
Closed

Batchinsert and associative arrays #61

SamMousa opened this issue Aug 10, 2018 · 12 comments
Milestone

Comments

@SamMousa
Copy link
Contributor

Clarification

There is more at play here than the "order of elements" argument:

  1. Associative arrays are conceptually like dictionaries. While they have an order in PHP, it is not intuitive to use to depend on it.
  2. In this bug specifically, if you use associative data rows then type casting will not working unless your columns array also uses the same indexes; this is even less intuitive.
    The columns parameter is the ordered list of columns, their indexes shouldn't matter.

Regardless of the argument that you can construct all kinds of arrays in PHP I propose documenting a preferred structure and actually checking if the code conforms...

What steps will reproduce the problem?

Create code like this:

$x->batchInsert('table1', ['a', 'b'], ['b' => 123, 'a' => 345]);

What is the expected result?

I expect it to work properly, ie create a query like this:

insert into `table1` (`a`, `b`) values (345, 123);

What do you get instead?

insert into `table1` (`a`, `b`) values (123, 345);

Solution

The thing that is wrong here is not so much the code as my assumption: batch insert does not support associative arrays; this means things like column order and also column type casting will go wrong when passing in associative arrays.

My proposal is to actively check each key in the row data and throw a hard exception if it's not numerical. Since we're already iterating we can do this without a significant performance cost; the only lines we'd add are something like this:

if (!is_int($i)) {
    throw new SomeKindOfException('Data rows must be numerically indexed');
}

Additional info

Q A
Yii version latest
PHP version N/A
Operating system N/A
Related issues #14608
@rob006
Copy link
Contributor

rob006 commented Aug 11, 2018

The problem is not associative array, but invalid order of elements. If you pass associative array with correct order, everything will work fine:

$x->batchInsert('table1', ['a', 'b'], ['a' => 345, 'b' => 123]);

Throwing exception will break perfectly fine working code.

@SamMousa
Copy link
Contributor Author

Associative arrays tend not to be ordered; I know they are but when using them you don't want to depend on the order of associative elements...

@rob006
Copy link
Contributor

rob006 commented Aug 11, 2018

And how this is different from arrays indexed by integers? You still can have [2 => 2, 1 => 1, 3 => 3].

@SamMousa
Copy link
Contributor Author

It's not, both of those are dictionaries; however php doesn't have lists so they are also dictionaries... These are conceptually different. My point not related to the order of elements BTW, in the correct order things like typecasting still don't work. Check the source if you don't believe me...

@paweljankowiak06
Copy link

I had similar issue and as simple solution I used keys from array as argument.

$array = ['b' => 123, 'a' => 345];
$x->batchInsert('table1', array_keys(reset($array)), $array);

This does not cover different keys order in every array but when every array has same keys order this should work. Also when you change/add/delete one of key this should still work.

@samdark
Copy link
Member

samdark commented Aug 19, 2018

Currently associative arrays have no meaning in batch insert. Keys are ignored. What's the use case for naming columns explicitly in each element of a huge data array?

@SamMousa
Copy link
Contributor Author

I don't have one, I just happened to get the data with keys. Then I found out the hard way that typecasting was broken because of it.

@samdark
Copy link
Member

samdark commented Aug 20, 2018

Well, then we can conclude that the format supplied wasn't supported and it was said nowhere in the docs that it would work. I see no reason to support such format in the future.

@samdark samdark closed this as completed Aug 20, 2018
@SamMousa
Copy link
Contributor Author

SamMousa commented Aug 20, 2018

@samdark You misunderstand me; I'm not proposing adding support, i'm proposing to make failure more obvious:

My proposal is to actively check each key in the row data and throw a hard exception if it's not numerical.

Since we're already iterating it's a cheap thing to do and will make non-apparent failures more obvious.

@samdark samdark reopened this Aug 20, 2018
@samdark
Copy link
Member

samdark commented Aug 20, 2018

Ah, good to go then.

@samdark samdark transferred this issue from yiisoft/yii2 Mar 10, 2019
@rob006
Copy link
Contributor

rob006 commented Mar 10, 2019

yiisoft/yii2#17066 (comment)

Tigrov added a commit to Tigrov/db that referenced this issue Aug 19, 2023
Tigrov added a commit to Tigrov/db-oracle that referenced this issue Aug 19, 2023
Tigrov added a commit to Tigrov/db-oracle that referenced this issue Aug 20, 2023
Tigrov added a commit to Tigrov/db that referenced this issue Aug 20, 2023
@Tigrov Tigrov closed this as completed in 39872b1 Nov 1, 2023
Tigrov added a commit to yiisoft/db-oracle that referenced this issue Nov 1, 2023
* Refactor DMLQueryBuilder

* Fix @psalm-var

* Add test for a non-primary key table

* Fix yiisoft/db#61 (point 2)

* Fix yiisoft/db#61 (point 2) add test

* improve test

* Add line to CHANGELOG.md

* Improve performance of quoting column names up to 10% using `array_map()`

* Remove `Generator`

* Update psalm
@Tigrov
Copy link
Member

Tigrov commented Nov 4, 2023

Point 2 was solved but point 1 still actual

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

No branches or pull requests

5 participants