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

Avoid calling DSL.nullSafe() and similar all over the DSL API, move the calls to QueryPart constructors #11091

Closed
lukaseder opened this issue Dec 4, 2020 · 3 comments

Comments

@lukaseder
Copy link
Member

All over the jOOQ API, we have DSL.nullSafe() and related calls that make sure that bind values are wrapped with the appropriate data type when writing things like this:

INT_COLUMN.eq(1);
CONVERTED_COLUMN.eq(new Something(...));

While we could guess that the value 1 is really just the same as DSL.val(1), in the second case, we cannot make any data type guesses. We have to use the type of CONVERTED_COLUMN. We can see, in AbstractField:

    @Override
    public final Condition equal(T value) {
        return equal(Tools.field(value, this));
    }

    @Override
    public final Condition equal(Field<T> field) {
        return compare(EQUALS, nullSafe(field, getDataType()));
    }

Both of these methods wrap the value using this.getDataType() information. It is being done twice, and then again in the compare() method:

    @Override
    public final Condition compare(Comparator comparator, Field<T> field) {
        switch (comparator) {
            case IS_DISTINCT_FROM:
            case IS_NOT_DISTINCT_FROM:
                return new IsDistinctFrom<>(this, nullSafe(field, getDataType()), comparator);

            default:
                return new CompareCondition(this, nullSafe(field, getDataType()), comparator);
        }
    }

And starting with jOOQ 3.15.0 also in the CompareCondition constructor because of the requirements here #11061:

    CompareCondition(Field<?> field1, Field<?> field2, Comparator comparator) {
        this.field1 = nullSafe(field1, field2.getDataType());
        this.field2 = nullSafe(field2, field1.getDataType());
        this.comparator = comparator;
    }

Doing all of this in the constructor has these benefits:

  • It is always the last call, so if we can delay wrapping and converting things, we will still be guaranteed of the conversion to happen
  • It is the most reasonable location where both expressions can be wrapped. Historically, we could convert types correctly when writing CONVERTED_COLUMN.eq(x), but not when writing val(x).eq(CONVERTED_COLUMN). This will be required by Parser should look up data types for bind variables from context #11061, and will now be possible when moving this logic into the constructors.
@lukaseder
Copy link
Member Author

A caveat of this change is that some constructor arguments are optional. When null they represent the absence of the argument, not the null bind value. This implementation detail shouldn't leak into the API, and users may have come to rely on a null field argument meaning null the bind value.

@lukaseder
Copy link
Member Author

This issue will be fixed step by step as an epic task along with #11061, #11070, #11091

lukaseder added a commit that referenced this issue Dec 4, 2020
lukaseder added a commit that referenced this issue Dec 4, 2020
lukaseder added a commit that referenced this issue Dec 7, 2020
lukaseder added a commit that referenced this issue Dec 7, 2020
- Use API code generator to generate function expression tree
implementations
lukaseder added a commit that referenced this issue Dec 8, 2020
lukaseder added a commit that referenced this issue Dec 8, 2020
There needs to be a mechanism to identify the data type of a function based on its default type and/or arguments. So far, there's a weak correlation of the behaviour and whether we're calling Tools::anyNotNull or Tools::allNotNull. Further refactorings will probably invalidate this correlation
lukaseder added a commit that referenced this issue Dec 8, 2020
lukaseder added a commit that referenced this issue Dec 8, 2020
lukaseder added a commit that referenced this issue Dec 8, 2020
lukaseder added a commit that referenced this issue Dec 8, 2020
lukaseder added a commit that referenced this issue Dec 8, 2020
Including CHAR_LENGTH, BIT_LENGTH, OCTET_LENGTH
@lukaseder
Copy link
Member Author

There will be more improvements in this area, which will be done via #11061 and #11070, implicitly

3.15 Other improvements automation moved this from To do to Done Dec 9, 2020
lukaseder added a commit that referenced this issue Dec 10, 2020
DSL::abs must maintain the input data type, not override it with the default INTEGER type
lukaseder added a commit that referenced this issue Dec 10, 2020
- COS
- COSH
- COT
- COTH
- SIN
- SINH
lukaseder added a commit that referenced this issue Dec 11, 2020
lukaseder added a commit that referenced this issue Dec 11, 2020
lukaseder added a commit that referenced this issue Dec 11, 2020
lukaseder added a commit that referenced this issue Dec 14, 2020
lukaseder added a commit that referenced this issue Dec 14, 2020
lukaseder added a commit that referenced this issue Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

1 participant