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

[5.2] Support composite column names like 'table.id' in chunkById #14499

Closed
wants to merge 1 commit into from
Closed

Conversation

mzur
Copy link
Contributor

@mzur mzur commented Jul 27, 2016

Example use case that didn't work before:

// both the annotations and images tables have an id column
DB::table('annotations')
   ->join('images', 'annotations.image_id', '=', 'images.id')
   ->where(/* something with images */)
   ->chunkById(500, $handleChunk, 'annotations.id');

As a workaround I used an alias for the ambiguous column:

DB::table('annotations')
   ->join('images', 'annotations.image_id', '=', 'images.id')
   ->where(/* something with images */)
   ->select('annotations.id as annotations_id', /* ... */)
   ->chunkById(500, $handleChunk, 'annotations_id');

But PostgreSQL for example doesn't allow the use of an alias in a where statement like it is built by chunkById.

mzur added a commit to biigle/reports that referenced this pull request Jul 27, 2016
@taylorotwell
Copy link
Member

I worry this will create very unexpected behavior. For example if you select two columns with the same name and the last one specified is the one taken, then use this chunk method in this way to access the other tables ID, you will ACTUALLY be getting an entirely different table's ID, which would be very unexpected. I actually think it it is best to be specific about what you want using an alias in select then trying to be all fancy and actually open the door for unexpected behavior.

@@ -1763,13 +1763,14 @@ public function chunkById($count, callable $callback, $column = 'id')
$lastId = null;

$results = $this->forPageAfterId($count, 0, $column)->get();
$columnSuffix = last(explode('.', $column));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't PHP throw a warning about not providing a reference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting point, one would certainly expect so. Apparently being wrapped in the helper function prevents the warning since this creates a new variable.

@mzur
Copy link
Contributor Author

mzur commented Aug 1, 2016

@taylorotwell Yes, you have a point. But it's not just trying to be fancy here, since my first example currently does not work (although one might expect it to) and my "workaround" with the alias only works with SQLite (as far as I know). Standard SQL doesn't allow a column alias in a where statement (e.g. Postgres, MySQL) but SQLite supports this in a non-standard way. The where statement is added by forPageAfterId.

What do you think of adding an optional fourth parameter to chunkById where the column alias can be defined? The alias will then be used instead of the $columnSuffix proposed by this PR which should not lead to unexpected behavior. Example:

DB::table('annotations')
   ->join('images', 'annotations.image_id', '=', 'images.id')
   ->where(/* something with images */)
   ->select('annotations.id as annotations_id', /* ... */)
   ->chunkById(500, $handleChunk, 'annotations.id', 'annotations_id');

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

Successfully merging this pull request may close these issues.

3 participants