custom type obliterated by from_statement

Issue #3935 wontfix
Eoghan Murray created an issue

Somehow the process_result_value is getting lost when from_statement is in use, see attached test case

Comments (8)

  1. Mike Bayer repo owner

    specifically, query.from_statement() means, "run this Core expression and get a result set back, then run that data into an entity, matching on name if no columns() are present" . Query does not run process_result_value, that's part of the Core result set. so your types have to be present in the core part of this.

  2. Eoghan Murray reporter

    To play devils' advocate, when the name match occurs, shouldn't the matched columns' process_result_value be called?

    (As I only came across this exploring solutions for #3933 I've no further active usecase for this)

  3. Mike Bayer repo owner

    the whole "name matching" thing is a hack in the first place, the newer concept of text() allowing direct column correspondence with the columns() method is better. that way you associate a full column w/ type up front.

    process_result_value on the ORM entity can't be unconditionally called because the Core expression might already have types in it that have done this step. An intricate way of checking would be to test the type of each column for "NullType" and then run it, but overall this goes against all the abstraction layers in place by asking the ORM to reach deep inside the internals of Core and run the type processors.

    short answer is that the expression passed into from_statement() should be linked to the columns one wishes to associate with directly, name matching is never the best way to go. it was a bad idea.

  4. Mike Bayer repo owner

    this does lead to another shortcoming in the API is that if you do have your Table that's against the view, how do we link those columns back to the original Table without using name matching. at least with aliased(v, Test.table, match_names=True) the name matching is between two Table objects and typing is not an issue, but this does suggest if we ever had a full blown "View" object, it might want to have the ability to relate its columns to other Table columns. There's ways to make this happen now but they require building custom objects, using a similar trick as _make_proxy() in https://bitbucket.org/zzzeek/sqlalchemy/wiki/UsageRecipes/Views .

  5. Eoghan Murray reporter

    One note from an API user pov; the existence of TextClause.columns is undiscoverable from looking at the docs for sql.expression.text maybe columns should be added as a keyword argument to the sql.expression.text method?

  6. Mike Bayer repo owner

    all the documentation has this thing where there's the "lowercase" function and you can click through to the Text() object to see the API, but also the .columns() method is demonstrated in the examples at http://docs.sqlalchemy.org/en/latest/core/sqlelement.html?highlight=text#sqlalchemy.sql.expression.text (" The TextClause.bindparams() method is used to provide bound parameter detail, and TextClause.columns() method allows specification of return columns including names and types:") It's also referred to in the description of the "typemap" argument which is deprecated. I'm not so into adding everything as kw arguments these days since it's cleaner to use method chaining. It's also in the ORM tutorial as mentioned earlier.

  7. Mike Bayer repo owner

    ill call this "wontfix" because i will grant that the behavior is unintuitive, and in general the area of relating two selectables to each other is still underdeveloped from a front-facing API perspective, but I don't think any changes should be made to the API here since there is a strict boundary between a Core selectable and how a Query interprets the rows from it.

  8. Log in to comment