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

Add Settings.renderAutoAliasedDerivedTableExpressions to auto alias unnamed field expressions in derived tables #579

Closed
lukaseder opened this issue Jul 22, 2012 · 11 comments

Comments

@lukaseder
Copy link
Member

lukaseder commented Jul 22, 2012

This will produce a syntax error:

create().select()
        .from(select(inline(1)))
        .fetch();

Workaround:

create().select()
        .from(select(inline(1).as("a")))
        .fetch();

See also:

@lukaseder
Copy link
Member Author

A thorough fix is best implemented when we have immutable queries, i.e. queries where we can no longer add columns to the SELECT clause. Because then, we can assign generated column names deterministically across queries and subqueries, without names ever changing. For instance:

Select<?> select =
create().select()
        .from(create().selectOne());

Field<?> field = select.field(0);

The above field reference would yield a generated field name (e.g. col1), and there is no way this name can ever change, from the above select reference.

@knutwannheden
Copy link
Contributor

This issue also cropped up in #1699 when testing the DB2 support.

@lukaseder lukaseder removed this from the Version 3.13.0 milestone Jan 20, 2020
@lukaseder
Copy link
Member Author

Looking into this now.

  • A good place to decide whether such aliases need to be generated is Alias
    • We'll do this only in the default implementation of toSQLWrapped
    • Only if there are no fieldAliases (which would provide explicit aliases already)
    • Only for Select or DerivedTable
  • The implementation is then in SelectQueryImpl, where we already have a mechanism of specifying select aliases

Since this is a delicate change with regression risks, I'm not going to backport the fix.

@lukaseder
Copy link
Member Author

A new marker interface is necessary to denote all the Field types that do not require such auto-aliasing, including e.g.:

  • FieldAlias
  • TableFieldImpl
  • Level
  • Rownum
  • SQLField (which may already contain an alias, we don't know)
  • UDTPathFieldImpl

@lukaseder
Copy link
Member Author

lukaseder commented Feb 22, 2024

Some extensive test will be necessary for all AbstractField implementations:

  • To ensure only those where we want auto-aliasing to apply are affected
  • To ensure that we cover all AbstractField implementations in a test (this meta test will fail for future implementations, by default)

@lukaseder
Copy link
Member Author

lukaseder commented Feb 22, 2024

More candidates:

  • Deleting (these are keywords, and thus don't work automatically)
  • FalseCondition
  • Inserting
  • NullCondition
  • SQLCondition
  • TrueCondition
  • Updating

Cases where a wrapper type should delegate to the contained type (similar to SimpleCheckQueryPart)

  • Coerce
  • ConditionAsField
  • FieldCondition
  • FieldProxy

In principle, there's also room for making this dialect specific, e.g. PI could be a NamedField in some dialects, but not in others. That might be a bit overengineered for now.

@lukaseder
Copy link
Member Author

lukaseder commented Feb 22, 2024

Very cool! This query:

ctx.selectFrom(select(T_BOOK.ID, inline(1), trueCondition()).from(T_BOOK))

Now renders this SQL:

select "alias_77757314"."id", "alias_77757314"."1", "alias_77757314".condition
from (
  select
    "public"."t_book"."id",
    1 as "1",
    true as condition
  from "public"."t_book"
) as "alias_77757314"

As can be seen, there are likely many AbstractCondition implementations that will now render the name condition, leading to ambiguities which weren't there before. I.e. this is a slight regression risk in case an RDBMS was able to derive a column name from an expression automatically, or in case anonymous columns are supported. For example in PostgreSQL:

select *
from (
  select
    true,
    false,
    1
) as t

Produces:

|?column?|?column?|?column?|
|--------|--------|--------|
|true    |false   |1       |

It's unclear if this only causes issues with the asterisk, currently, because it's not really possible to dereference any columns from t

@lukaseder
Copy link
Member Author

Another regression risk, our Field.getName() values may conflict with keywords, e.g. this query is now being generated for a test in MySQL:

insert into `test`.`t_author` (`ID`, `LAST_NAME`)
values (
  (
    select *
    from (
      select (max(`test`.`t_author`.`ID`) + 1) as add
      from `test`.`t_author`
    ) as `t`
  ), 
  'X'
)

The error is now:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'add from test.t_author) as t), 'X')' at line 1

This works:

insert into `test`.`t_author` (`ID`, `LAST_NAME`)
values (
  (
    select *
    from (
      select (max(`test`.`t_author`.`ID`) + 1) as `add`
      from `test`.`t_author`
    ) as `t`
  ), 
  'X'
);

@lukaseder lukaseder changed the title Cannot use unnamed field expressions in subqueries Auto alias unnamed field expressions in derived tables Feb 22, 2024
@lukaseder lukaseder changed the title Auto alias unnamed field expressions in derived tables Add Settings.autoAliasDerivedTableExpressions to auto alias unnamed field expressions in derived tables Feb 26, 2024
@lukaseder
Copy link
Member Author

lukaseder commented Feb 26, 2024

Continuing this, I feel there are many risks of introducing this as a new default behaviour, especially when asterisks are involved, i.e. when name resolution in the RDBMS is not being done.

For example, the following transformations are now made:

-- Before
select * from t, (select 1) u

-- After
select * from t, (select 1 "1") u

But depending on other settings (e.g. quoting of identifiers), "1" may be unquoted, in case of which it's an invalid identifier again, just like add before.

If, however, we introduce this as an opt-in behaviour, guarded by a flag called Settings.autoAliasDerivedTableExpressions, then the feature might be acceptable. The flag could have 3 states:

  • NEVER (default, status quo)
  • UNNAMED
  • ALWAYS

@lukaseder lukaseder changed the title Add Settings.autoAliasDerivedTableExpressions to auto alias unnamed field expressions in derived tables Add Settings.renderAutoAliasedDerivedTableExpressions to auto alias unnamed field expressions in derived tables Feb 26, 2024
@lukaseder
Copy link
Member Author

The 3 state setting might be useful in the future, when we do this also at the top level to guarantee field names (I think there's already an issue for this).

Implemented in jOOQ 3.20

3.20 Other improvements automation moved this from To do to Done Feb 26, 2024
lukaseder added a commit that referenced this issue Feb 26, 2024
lukaseder added a commit that referenced this issue Feb 26, 2024
Like SimpleCheckQueryPart for SimpleQueryPart, a new NamedCheckField for NamedField contents has been added for cases like Coerce, which are wrappers for something that could be a NamedField.
@lukaseder
Copy link
Member Author

Worth looking into the SPI for auto alias generation as well:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment