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

Scalar::Defer support as column name #98

Closed
wants to merge 1 commit into from
Closed

Scalar::Defer support as column name #98

wants to merge 1 commit into from

Conversation

x86-64
Copy link
Contributor

@x86-64 x86-64 commented Mar 30, 2016

So then using Scalar::Defer (or any other defer module) object as column name in search(), code call ref on it, in case of Scalar::Defer ref eq "0", and if we check length it will be == 1, so condition is false and it falls to "else" condition, which treat defer object as subject for serialize. However, serialize is using Storable::freeze, which has a line "logcroak "not a reference" unless ref($self);" and for defer object that is true, so it croaks.
I also tried to re-bless it into different package, not "0", but it only fails in different place instead. This looks like best solution for me, would be better if there was any certain way of knowing if object can be stringified and use that, but I don't see it yet.

@ribasushi
Copy link
Collaborator

Hi!

I need to understand your failure case before I can say anything about the patch.

Please show me how exactly are you using Scalar::Defer within a search() for this to bite you.

Thanks!

@x86-64
Copy link
Contributor Author

x86-64 commented Mar 30, 2016

Well I'm using it to work with joins in dbix-class, looks like this:

use DBIx::Class::Helper::XJoin;
my $rs = $schema->resultset('Project');
xjoin($rs,                 
    "parent.tags as tag1",        
    "parent.tags as tag2",        
    "tags as tag3", 
);
$rs->search([
    xjoin_column_alias("parent.tag1.name") => "Open",
    xjoin_column_alias("parent.tag2.name") => "Public",
    xjoin_column_alias("tag3.name")        => "Private",
]);

xjoin_column_alias returns object from Scalar::Defer, so it basically boils down to:

  use Scalar::Defer qw/defer/;
  my $rs = $schema->resultset('Project');
  $rs->search([
       defer { "column" } => "value",
  ]);

Does it make any sense?

@ribasushi
Copy link
Collaborator

I now understand what your problem is. It's not that the thing doesn't work, but that you get defer() called too many times.

For starters the proposed fix is a no-go: the codebase in general goes to great lengths to not treat bless into false in a special way. In fact any naked ref() in boolean context is a mistake, not the other way around. But there is another reason this can not be applied:

The more fundamental problem is that SQL::Abstract fundamentally does not support your desired use-case (which you already ran into here ...I also tried to re-bless it into different package, not "0", but it only fails in different place...):

~$ perl -MSQL::Abstract -MScalar::Defer -MData::Dumper -e '
  my $x = my $y = 0;
  print Dumper [ map { "$_" }
    SQL::Abstract->new->where([
      defer { ++$x }, defer { ++$y }
    ]) ]
'

$VAR1 = [
          ' WHERE ( 2 = ? )',
          '2'
        ];

While it is possible to get things to work in the long run (with a ton of work), a small one-off patch like what is proposed in this PR is certainly off the table.

Could you describe the actual problem you are trying to solve? Perhaps there is a better way forward without using (the clearly not working OOB) Scalar::Defer at all.

@x86-64
Copy link
Contributor Author

x86-64 commented Apr 1, 2016 via email

@ribasushi
Copy link
Collaborator

As for SQL::Abstract doesn't seem like an issue, defer will return same result over and over

...? I just demonstrated in that oneliner that the thing is evaluated twice, not once.

Main goal was to solve issues with joins. I use chained resultsets a lot, and they often go into ResultSet classes as helpers like "with_tag", "with_parent_tag"

Right. The correct answer to this is proper alias option support for join specifications, which has not yet been implemented due to various hurdles. I have not revisited this yet, and can not currently say for sure if I will be able to cover this before I depart.

I guess they (join specs) should be sticky.

They can't be because the join spec is calculated lazily at the end of a callchain, ensuring proper deduplication can take place (e.g. search({}, { join => 'foo')->search({}, { join => 'foo'}). This does have the unintended effect of the entire thing "shifting" from under you when an identically named join is added on a distinct yet parallel relation branch. It is a limitation on the original design, and various deployments depend on various quirks of it, hence a large scale "sanity fix" is not on the cards (even if attributes are implemented, there still would likely be no system to avoid clashes).

There is however a separate technique allowing you to sidestep the problem entirely: use correlated subqueries. The concept is more or less the same but it is both less fragile (each subquery is an implicit namespace and clashes can not occur) and more efficient (most optimizers, including MySQL's, recognize that the join does not need to be reified).

You can either do this by hand, or use @frioux's helper to achieve the same effect as described in the linked SYNOPSIS.

If this doesn't work for you - you will have to give up chaining for the time being, and build the structure separately from DBIC, only feeding it the final monolithic where+join criteria. The tooling is just not up to the task yet.

Even though I am closing this PR due to the considerations discussed higher up, feel free to add more thoughts/questions to this ticket if the above does not give you a satisfactory set of answers.

Cheers!

@ribasushi ribasushi closed this Apr 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants