kasei / perlrdf

Deprecated in favor of the Attean package
26 stars 25 forks source link

RDF::Trine::Store::SPARQL::_group_bulk_ops doesn't build array correctly #146

Closed minusdavid closed 7 years ago

minusdavid commented 7 years ago

In RDF::Trine 1.016, RDF::Trine::Store::SPARQL::_group_bulk_ops produces an @aggops array that contains an arrayref where the 0th element is the type (e.g. '_add_statements'), and the 1st element is an array (which has a quad for the 0th element, undef for the 1st element, and then a series of arrays containing a quad and a context as expected).

So that's weird...

We iterate through @aggops, and dreference $aggop into ($type,@ops).

This creates another problem. @ops should probably be $ops there, because it seems we've now put an arrayref within an array.

So when we iterate through @ops, we only get one $op and $op->[0] gives us $st and $op->[1] gives us $context... but only for 1 operation. All the other operations are lost because they're part of this arrayref living in this single $op.

If we did: my ($type, $ops) = @$aggop;

Instead of: my ($type, @ops) = @$aggop;

Then we'd have an arrayref with many more operations, but it wouldn't work because the first 2 elements are a RDF::Trine::Statement::Quad object and undef. Those two elements should be wrapped up into an arrayref as well.

So ultimately... I think RDF::Trine::Store::SPARQL::_group_bulk_ops isn't creating @aggops correctly during RDF::Trine::Store::SPARQL::_end_bulk_ops, and $aggop isn't being dereferenced correctly in RDF::Trine::Store::SPARQL::_end_bulk_ops.

The problem is this line: push(@bulkops, [$type, [ @{$op}[1 .. $#{ $op }] ]]);

It should be: push(@bulkops, [$type, [ [ @{$op}[1 .. $#{ $op }] ]]]);

That also means the later line: push(@bulkops, [$type, [ @{$op}[1 .. $#{ $op }] ]]);

Should be: push(@bulkops, [$type, [[ @{$op}[1 .. $#{ $op }] ]]]);

I think this code could actually be a bit cleaner, but it's 8:17pm on a Tuesday, so I think a pull request might have to wait until tomorrow.

minusdavid commented 7 years ago

Marking this as resolved now.