Need a convenience method to add group of SqlCriterion directly

[JoshuaJeme]

When building query for a query param, conditions may be optional. It's hard to determind which condition should be use as the initial one.

Assume that we have a query like (cond1 = 1 or cond2 = 2) and cond3 = 3 and a param class below.

static class Param {
    Integer cond1;
    Integer cond2;
    Integer cond3;
}

cond1/2/3 are all optional, if absent, the condition will be ignored.

In this case, SqlCriterion and(BindableColumn<T> column, VisitableCondition<T> condition, SqlCriterion...subCriteria) is not so convenience.

If we provide a way that can add list of conditions without specifing the initial one, it will be much better.

List<SqlCriterion> conditions = new LinkedList<>();
QueryExpressionDSL<SelectModel> dsl = select(column1, column2).from(table);

QueryExpressionWhereBuilder builder = dsl.where();
Optional.ofNullable(param.cond1).map(c -> or(column1, isEqualTo(c))).ifPresent(conditions::add);
Optional.ofNullable(param.cond2).map(c -> or(column2, isEqualTo(c))).ifPresent(conditions::add);
builder.and(conditions);

if (null != param.cond3) {
    builder.and(column3, isEqualTo(param.cond3));
}

SelectStatementProvider selectStatement = dsl.build().render(RenderingStrategies.MYBATIS3);

[jeffgbutler]

This solution seems quite a lot more complex than using what is already supplied in the library:

SelectStatementProvider selectStatament = select(column1, column2)
  .from(table)
  .where(column1, isEqualToWhenPresent(param.cond1), or(column2, isEqualToWhenPresent(param.cond2)))
  .and(column3, isEqualToWhenPresent(param.cond3))
  .build()
  .render(RenderingStategies.MYBATIS3);

[JoshuaJeme]

Thanks for your reply @jeffgbutler

The code is much more clean when using *WhenPresent.

SelectStatementProvider selectStatement = select(column1, column2)
    .from(table)
    .where(or(column1, isEqualToWhenPresent(param.cond1)), or(column2, isEqualToWhenPresent(param.cond2)))
    .and(column3, isEqualToWhenPresent(param.cond3))
    .build().render(RenderingStrategies.MYBATIS3);

I still think it's necessary to provide a way to add group of conditions.

Conditions in the same parentheses are equally, current API treat the rest conditions as the children(subCriteria) of the first one. It's not so natural and need to get used to it.

CriterionGroup can solve case like ((A = 1 or A > 5 ) and B = 1 or (A < 0 or A = 4) and B = 2) and C = 1 easily without considering how to organize the condition.

Another case is, when we have an existing list of SqlCriterion (eg. Pass in as arg), it can be append into the WhereBuilder directly through CriterionGroup.

Feature like CriterionGroup may has been considered before, I'd like to know the opinion of you, looking forward to your reply.

[JoshuaJeme]

For query like ((A = 1 or A > 5 ) and B = 1 or (A < 0 or A = 4) and B = 2) and C = 1

Current API:

Current API mast specific column and condition
SqlCriterion and(BindableColumn<T> column, VisitableCondition<T> condition, SqlCriterion...subCriteria).
We have to reorganize the condition to (B = 1 and (A = 1 or A > 5 ) or (A < 0 or A = 4) and B = 2) and C = 1.

select(a)
.from(table)
.where(b, isEqualTo(1)
    , and(a, isEqualTo(1), or(a, isGreaterThan(5)))
    , or(a, isLessThan(0), or(a, isEqualTo(4)))
    , and(b, isEqualTo(2))
)
.and(c, isEqualTo(1))

CriterionGroup Only:

Conditions can be group in parentheses directly without reorganization.

select(a)
.from(table)
.where(
    and( // connector 'and' is useless
            // the 'or' after it is unnecessary too
        or(a, isEqualTo(1))
        , or(a, isGreaterThan(5))
    )
    , and(b, isEqualTo(1))
    , or(
        or(a, isLessThan(0))
        , or(a, isEqualTo(4))
    )
    , and(b, isEqualTo(2))
)
.and(c, isEqualTo(1))

Current API with CriterionGroup:

Use current API to reduce unnecessary connector.

select(a)
.from(table)
.where(
    // Much better, but still has a useless 'and'
    and(a, isEqualTo(1), or(a, isGreaterThan(5)))
    , and(b, isEqualTo(1))
    , or(a, isLessThan(0), or(a, isEqualTo(4)))
    , and(b, isEqualTo(2))
)
.and(c, isEqualTo(1))

[jeffgbutler]

Thanks for the detailed explanation. I understand the problem with the initial parenthesis - we don't have a way to deal with that currently.

I'm reviewing your pull request for this and will make some comments there. One thing I'd like you to change is to use a method name of "group" for this new function rather than reusing "and" and "or". I think it will help to lower the confusion about this. So your example would look like this:

select(a)
.from(table)
.where(
    group(a, isEqualTo(1), or(a, isGreaterThan(5))),
    and(b, isEqualTo(1)),
    or(a, isLessThan(0),
    or(a, isEqualTo(4))),
    and(b, isEqualTo(2))
)
.and(c, isEqualTo(1))

[jeffgbutler]

I added quite a few comments to your PR. I've taken a slightly different approach in my experimenting with this. If you want to look at my branch to see the differences, you can look here: https://github.com/jeffgbutler/mybatis-dynamic-sql/tree/gh-430

[JoshuaJeme]

Thanks for the detailed explanation. I understand the problem with the initial parenthesis - we don't have a way to deal with that currently.

I'm reviewing your pull request for this and will make some comments there. One thing I'd like you to change is to use a method name of "group" for this new function rather than reusing "and" and "or". I think it will help to lower the confusion about this. So your example would look like this:

select(a)
.from(table)
.where(
    group(a, isEqualTo(1), or(a, isGreaterThan(5))),
    and(b, isEqualTo(1)),
    or(a, isLessThan(0),
    or(a, isEqualTo(4))),
    and(b, isEqualTo(2))
)
.and(c, isEqualTo(1))

Happy new year and sorry for my late reply as I am on my holiday.
group is better, I'd thinked about it before. Others may think it has relation with groupby on first sight. But I can't find other better method name.
parenthesis/bracket are not good too.

[djechelon]

I am experimenting with it and looks like it mostly satisfies my needs. Mostly because it's not possible (or I don't know how) to merge an unknown list of conditions into a single group, except by adding a dummy "1=1"/"1=0" condition (latter being painful).

I have a runtime filter stacking AND conditions. Okay. But some conditions are in OR and must be grouped. They can be zero, one or many.

@Override
    public WhereApplier applyWhereStatement(MybatisFilterFramework filterFramework, SqlTable entityTable) {
        VwControlVerificationDynamicSqlSupport.VwControlVerification vwControlVerification = (VwControlVerificationDynamicSqlSupport.VwControlVerification) entityTable;
        ExistsPredicate domainExists;
        SqlCriterion workflowAndLegalEntity;

        if (amlDomain != null)
            domainExists = exists(
                    select(StringConstant.of("1"))
                            .from(taxonomyDomain, "linkedDomain")
                            .join(amlRequirement, "linkedRequirement").on(taxonomyDomain.idelement, equalTo(amlRequirement.requirementdomainid))
                            .join(amlObjectLink, "requirementLink").on(amlObjectLink.objectidlinked, equalTo(amlRequirement.requirementid)).and(amlObjectLink.objecttypeidlinked, equalTo(amlRequirement.requirementtypeid))
                            .applyWhere(filterFramework.applyFilter(dsl -> dsl
                                            .where(amlObjectLink.objectid, isEqualTo(vwControlVerification.verificationcontrolid))
                                            .and(amlObjectLink.objecttypeid, isEqualTo(AmlObjectType.CONTROL.getValue()))
                                    , taxonomyDomain.idelement, Short.class, amlDomain))
            );
        else domainExists = null;

        if (segregation != null && !segregation.isEmpty()) {
            AndOrCriteriaGroup[] criteriaGroup = segregation.stream()
                    .filter(x -> x.getLegalEntityId() != null || x.getWorkflowType() != null)
                    .map(seg -> or(group(vwControlVerification.legalentityid, isEqualToWhenPresent(seg.getLegalEntityId()), and(vwControlVerification.controlworkflowtype, isEqualToWhenPresent(seg.getWorkflowType())))))
                    .toArray(AndOrCriteriaGroup[]::new);

            if (criteriaGroup.length > 0)
                workflowAndLegalEntity = group(StringConstant.of("1"), isEqualTo(StringConstant.of("0")), criteriaGroup);
            else workflowAndLegalEntity = null;
        } else
            workflowAndLegalEntity = null;

        return row -> {
            if (domainExists != null)
                row.and(domainExists);
            if (workflowAndLegalEntity != null)
                row.and(workflowAndLegalEntity);
        };
    }

Segregation is a pair of (enum workflowType, Set<Short> legalEntityId). My ultimate goal is

SELECT * FROM AML_VW_ControlVerifications
WHERE
            ControlStatus = ? --Not shown in the code
    AND ControlYear = ? --Not shown in the code
    AND ....... -- Other conditions
    AND (
                 (ControlWorkflowType = ? AND ControlLegalEntityId IN ?)
         OR   (ControlWorkflowType = ? AND ControlLegalEntityId IN ?)
         OR   (ControlWorkflowType = ? AND ControlLegalEntityId IN ?)
            )

In particular, if the Set is empty, the whole braced condition should be blanked out.

In short, I am asking if there is a way to build a group() out of a list of conditions, other than applying a first condition and then an unbounded list of sub-criteria.

Shorter example

The following example is perfectly equal to the SQL IN statement, but please consider it as an example.

Suppose I have Collection<Integer> columnValues, I want to generate something like

(Column = ? OR Column = ? OR Column = ?)

for any number of elements in the collection, in particular when there is only one value available in the collection.

[jeffgbutler]

@djechelon I think I understand the issue. I've built out a test and made a few changes to support this. The test is in my branch here: https://github.com/jeffgbutler/mybatis-dynamic-sql/blob/7b5881129b2850b11b4b6d39c93f1f30a3b28866/src/test/java/issues/gh430/NoInitialConditionTest.java

Would you please review and make sure this will work for your needs? Note this is not fully implemented for WHERE/AND/OR/NOT/GROUP yet - but if this is an accurate description of the issue then it will be easy to complete the implementation.

Thanks!

[djechelon]

I have just checked the test case as it gives me a full insight of the new API. It is exactly what I was looking for.

In particular, I believe that the criteria can be built out of Java streams or for-each loops just fine.
Of course, line 45 of the test is misleading, but it's fine for what I require. There is no point in using an OR operator for the first element when you add elements statically to the list. But it's powerful when you construct the list out of a for-each loop or Stream.

I haven't checked the implementation you may have or not done in the branch. The interface looks great from the tests you linked.

Thanks for your time!

[jeffgbutler]

Cool. That test is working in the branch. The basic rendering infrastructure was already mostly there so it didn't require much beyond some new API methods. I need to add similar support for WHERE/OR/NOT/GROUP, and make it a bit easier in Kotlin. That won't be too much trouble.

I agree with your comment about line 45 - makes it easier with a stream transform, but not necessary when statically constructing criteria. Almost every condition can be marked as non-renderable through a filter method, so we already have support for dropping the first "and"/"or" and dropping parentheses if there is only one item in a list. This just takes advantage of support already in the library.