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

DATAJPA-1435 - change COMPLEX_COUNT_VALUE for native query pageable sql. #381

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@JIANGc
Copy link

commented May 31, 2019

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).
@pivotal-issuemaster

This comment has been minimized.

Copy link

commented May 31, 2019

@JIANGc Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented May 31, 2019

@JIANGc Thank you for signing the Contributor License Agreement!

@JIANGc

This comment has been minimized.

Copy link
Author

commented May 31, 2019

A native query ex.

SELECT a.id AS id, a.time AS createTime, a.item AS item, a.category AS category, a.biz AS business, a.c_type AS contentType, a.cp_api AS cpApi, a.dedup AS dedup, a.content_level AS contentLevel, a.reserve_cp_api AS reserveCpApi, SUM(b.value) AS dedupCount, SUM(c.value) AS originalCount FROM statis_data AS a LEFT JOIN statis_data AS b ON a.id = b.id AND b.dedup = 1 LEFT JOIN statis_data AS c ON a.id = c.id AND c.dedup = 0 WHERE (COALESCE(:contentLevel) is NULL OR a.content_level IN (:contentLevel)) AND (COALESCE(:contentType) is NULL OR a.c_type IN (:contentType)) AND (COALESCE(:contentAppId) is NULL OR a.biz IN (:contentAppId)) AND (COALESCE(:cpApi) is NULL OR a.cp_api IN (:cpApi)) AND a.category = 'total' AND a.time BETWEEN :startTime AND :endTime AND a.item = 'content_accept_num' GROUP BY a.cp_api, a.c_type, a.content_level, a.biz

then after this:
countQuery = matcher.replaceFirst(String.format(COUNT_REPLACEMENT_TEMPLATE, replacement));

we have :
countQuery = "select count(a) FROM statis_data AS a LEFT JOIN statis_data AS b ON a.id = b.id AND b.d ... ..."

$3$6 in regex get the table name. So query "select count(tableName) ... ..." not works

@JIANGc

This comment has been minimized.

Copy link
Author

commented May 31, 2019

@mp911de any hope for this one getting merged? Thank you for the review.

@schauder

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

I'll try to look into this next week.
Since the change is surprisingly small chances aren't to bad for it.
The challenges will be:

  • does it actually fix the problem? This is currently hard to judge because there is no testcase reproducing the original problem. Although the description of the issue might be detailed enough to create one.
  • I'll have to think hard if it might break anything.

jiangchao1 added some commits Jun 2, 2019

@JIANGc

This comment has been minimized.

Copy link
Author

commented Jun 2, 2019

I found that this code will have problem if there is distinct in sql.

Another solution is that we set
private static final String COMPLEX_COUNT_VALUE = "$3$4";

and use sub queries in From clause. In this case we can keep distinct, all functions and all column names.

@schauder

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I like the subquery approach. Sounds less brittle than the current "Take that SQL statement".
The problem with that though is that JPQL doesn't support subqueries in the FROM-clause as far as I know and interpret the specification.

An actual test case would be very helpfull, because right now I don't even know what the exact problem is.

@JIANGc

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

Thanks @schauder. Indeed. I submitted an update. JPQL and SQL can be parsed separately now.
I added also some test cases. Hope this can be helpful.

schauder added a commit that referenced this pull request Jun 20, 2019

DATAJPA-1435 - Fixed Count query for native queries.
Native queries now use a subselect for the count query, getting rid of a lot of parsing.

Original pull request: #381.

schauder added a commit that referenced this pull request Jun 20, 2019

DATAJPA-1435 - Refactoring.
Moved the `isNative` argument from methods to the constructor, reestablishing backward compatibility and keeping data together that belongs together.
Applied some polishing.

Original pull request: #381.
@schauder

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I refactored the changes because they broke backward compatibility.
And I also found a case where this breaks: an order by clause doesn't get properly removed.

The refactored version including a failing test is on branch pr/datajpa-1435

@JIANGc

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

Indeed. The regex remove all characters after order by clause. I made some changes to fix this. I also create another pull request(DATAJPA-1435 - fix order by clause problem) to branch pr/datajpa-1435.

@schauder schauder referenced this pull request Jun 24, 2019

Open

DATAJPA-1435 - fix order by clause problem #385

5 of 5 tasks complete
@schauder

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Superseded by #385.

@schauder schauder closed this Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.