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

RHS path support (Ad Hoc JOIN)? #15

Open
wwadge opened this issue May 18, 2016 · 20 comments
Open

RHS path support (Ad Hoc JOIN)? #15

wwadge opened this issue May 18, 2016 · 20 comments
Assignees
Labels
Milestone

Comments

@wwadge
Copy link
Collaborator

wwadge commented May 18, 2016

Hi,

Is it possible to have an expression like employee.company_id == company.id ?

i.e. having a path on both sides of the expression?

@vineey
Copy link
Owner

vineey commented May 19, 2016

Are you referring to join statement between two tables? sorry this kind of statement is not supported yet but a milestone. It would not be part of the rql filter expression.

In our practice, we define the table relationship in the backend by providing a JOIN PATH MAP such as
the code below. These must be annotated with jpa associations(ManyToOne, OneToMany,...).

Map<Path, EntityPath> JOIN_PATH = ImmutableMap.<Path, EntityPath>.builder()
         .put(QEmployee.employee.company, QCompany.company)
         .build();

This will be part of 1.1.0 release in the future, tentatively on end of June or early July this year.
This May, the priority is supporting querydsl v4.x.x as simultaneous 2.0.0 release which requires migration of the current 1.0.0 release which supports 3.x.x.

Along with these, I will verify if another version, which supports arbitrary join columns , could be implemented in a way like the code below.

Map<Path, Path> JOIN_PATH = ImmutableMap.<Path, EntityPath>.builder()
         .put(QEmployee.employee.companyId, QCompany.company.id)
         .build();

This is fully supported by the latest hibernate version (5.x) and of course querydsl v4.x.x, thus this version of join statement could be part of rsql-queyrdsl v.2.x.x.RELEASE. Not sure yet if we can backport this to querydsl v3.x.x, hence rql-querydsl v1.x.x.RELEASE

@vineey vineey added the feature label May 19, 2016
@vineey vineey added this to the 1.1.0.M1 milestone May 19, 2016
@vineey vineey self-assigned this May 19, 2016
@vineey
Copy link
Owner

vineey commented May 19, 2016

@wwadge If you want to contribute, please let me know. Any help is greatly appreciated, and it will be posted in the documentation and release notes. We can discuss the implementation plans and decide how we can divide and assign the tasks.

Meanwhile, please feel free to explore the code you have forked. Thanks.

@wwadge
Copy link
Collaborator Author

wwadge commented May 19, 2016

I would like to contribute. I was thinking something along these lines -- it creates a correct predicate at least. Wouldn't that work?

 @Override
    public BooleanExpression evaluate(BooleanPath path, ComparisonNode comparisonNode, QuerydslFilterParam param) {
        Object arg = comparisonNode.getArguments().get(0);

        ComparisonOperator operator = comparisonNode.getOperator();

        if (arg != null) {
            Path rhsPath = param.getMapping().get(arg);

            if (EQUAL.equals(operator)) {
                return path.eq(rhsPath);
            }

            if (NOT_EQUAL.equals(operator)) {
                return path.ne(rhsPath).or(path.isNull());
            }
        }


        Boolean barg = convertToBoolean(comparisonNode);


        if (barg == null) {
            return path.isNull();
        }


        if (EQUAL.equals(operator)) {
            return path.eq(barg);
        }

        if (NOT_EQUAL.equals(operator)) {
            return path.ne(barg).or(path.isNull());
        }

        throw new UnsupportedRqlOperatorException(comparisonNode, path.getClass());
    }

I will probably also migrate to querydsl 4.x though I see there's already a branch named feature/querydsl4

@vineey
Copy link
Owner

vineey commented May 19, 2016

@wwadge Awesome! Your idea just make sense, though we still have to validate explicitly if the paths(columns) involve have the same data type. This kind of feature can support dynamic reporting that doesn't involve predefined table relationship. To support this we should provide a way to discover the tables involved in this filter expression and automatically include them in the querydsl FROM clause. Let me think about this one, will analyze the impact and possible issues that could arise. You can also provide a working POC if you want.

As for this project, my original plan is to support predefined table relationship, which I said last time. Using this predefined relationship via joined path map and defining the fields to be retrieved via the select field map, the querydsl JOIN clause is built base on these parameters. So basically, I don't use the WHERE clause for relationship. Also in this way, the backend can control which fields and relationships are allowed for the client to be used, for security purposes.

@vineey
Copy link
Owner

vineey commented May 19, 2016

@wwadge If you want to contribute to 2.0.0.RELEASE, you can start the migration of the code from querydsl v3.x.x to querydsl v4.x.x on the feature/querydsl4 branch. Later, just send me a pull request and I will review it. What do you say?

@wwadge
Copy link
Collaborator Author

wwadge commented May 19, 2016

ok let me update to querydsl4 first and submit a PR

@wwadge
Copy link
Collaborator Author

wwadge commented May 19, 2016

PR submitted: #16

@vineey vineey changed the title RHS path support? RHS path support (Ad Hoc JOIN)? May 21, 2016
@vineey
Copy link
Owner

vineey commented May 21, 2016

@wwadge hibernate 5.1 now supports ad hoc join on entities with no explicit association mapping.
Please refer to this link,

https://in.relation.to/2016/02/10/hibernate-orm-510-final-release/

https://blog.anthavio.net/2016/03/join-unrelated-entities-in-jpa.html

This can be taken advantage by rsql-querydsl 2.x.x.RELEASE. But I'm thinking instead of mixing it with rsql filter expression which should just contains lhs path and rhs values, we could provide a separate kind rsql expression just for joining two tables. This will add modularity and clean code since this requires a total different implementation.

Also, in this way, we could still provide a HashMap that would define the allowable ad hoc join columns between tables, thus the backend can still have the restrictions. On the other hand, not defining the HashMap means allowing the client to join any columns between known tables.

What do you say?

@vineey
Copy link
Owner

vineey commented May 21, 2016

With regards to 1.x.x.RELEASE, a different approach needs to be taken. Possibly won't support it since hibernate used by querydsl 3.x.x doesn't support it directly. Though there is still a way to do it, via subquery. A subquery is the only way to support ad hoc table relationship using an exist expression in the main query WHERE clause and the ad hoc join defined in the subquery WHERE clause.

@vineey vineey modified the milestones: 2.1.0, 1.1.0 May 21, 2016
@wwadge
Copy link
Collaborator Author

wwadge commented May 21, 2016

I think it's best to support both options, because for this join you would only get == (equality) no?

Let's say you have 2 tables and your desired filter expression is something like:
where (tableA.creationTimestamp > tableB.creationTimestamp)

that would be hard to express if I'm not allowed to refer to a specific field in RHS.

Also IMHO It's not worth bothering with querydsl 3 + hibernate 4 any more, there's always your v1 branch for that if need be. Spring Boot for eg have already shifted to hibernate 5 and will drop support for v4 after spring boot 1.4

@vineey
Copy link
Owner

vineey commented May 21, 2016

I agree, not worth supporting older version.

With regards to a different comparison operator between two tables, I've never tried to join tables using an operator other than equality. Correct me if I'm wrong, normally when joining two tables we have this idea of combining them via two columns with equal operator(==) and then additional filtering on other columns that can use other operators(<, <=, >=, <>). I can't see the sense of joining two tables via LT or GT operators, though theoretically it should be possible. Thoughts?

@wwadge
Copy link
Collaborator Author

wwadge commented May 21, 2016

Here's a use case:

tableA{
   id   INTEGER
   parent_id   FK link to tableB's id
   timestampA DATE
}

tableB{
    id INTEGER
   someOtherField text
   timestampB date
}

so I want to join the two together via parent_id link with a normal join using = but then I want to be able to say, I only care about the records in table A that are not older than tableB

i.e. I want something like:
WHERE timestampA <= timestampB
JOIN tableB b ON a.parent_id = b.id

@vineey
Copy link
Owner

vineey commented May 21, 2016

@wwadge Ok, I see, this is a valid scenario. This will required lhs and rhs path support. But this also means we still need joining columns, so the example you have provide will still produce results containing records of Table A and doesn't include Table B's column in the result. Table B is only used for filtering on this case.Is this right?

@wwadge
Copy link
Collaborator Author

wwadge commented May 21, 2016

Yes that is correct, that's why I said I think both features are required here. RHS path support is fairly easy and I'll do that if you want, but I'm not sure where to start with joins.

@vineey
Copy link
Owner

vineey commented May 21, 2016

Yes joins are not yet introduced in the recent release but is part of this next milestone. We should consider the scope of customization on join statements such as how to declared the:

  • kind of JOIN (LEFT, INNER, RIGHT),
  • allowed columns to be joined

The select, sort and filter expression should be considered as factors which join columns to include in the expression when building it, since you can only include columns of tables that are part of the join.

@vineey
Copy link
Owner

vineey commented May 21, 2016

see #20

@vineey vineey assigned wwadge and unassigned vineey May 21, 2016
@vineey
Copy link
Owner

vineey commented May 21, 2016

Let me finish #20, and you can proceed with this ticket. Before the end of May, hopefully I can share you the code so you can start implementing rhs. Sounds good?

@wwadge
Copy link
Collaborator Author

wwadge commented May 21, 2016

Yep, great; thanks.

@vineey
Copy link
Owner

vineey commented May 30, 2016

@wwadge Hi wadge, issue #19 and #20 has been implemented and code merged into develop branch. Also I've already created a branch for this ticket namely, feature/RHS, where you can proceed.

@vineey
Copy link
Owner

vineey commented May 30, 2016

@wwadge I suggest take a look into PathToValueConverter and PathToPathConverter and see the difference. The former one was the original implementation that supports only LHS paths and RHS values. The PathToPathConverter is an interface that supports RHS path. This way we could separate them and make a cleaner code. Please let me know if you have a better idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants