You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by olafgarten <gi...@git.apache.org> on 2016/08/15 10:54:00 UTC
[GitHub] metamodel pull request #124: Added code to remove TIMESTAMP tokens from SQLS...
GitHub user olafgarten opened a pull request:
https://github.com/apache/metamodel/pull/124
Added code to remove TIMESTAMP tokens from SQLServer queries
SQL Server doesn't allow for the TIMESTAMP token in where clauses.
Related to JIRA Issue METAMODEL-1112
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/olafgarten/metamodel master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/124.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #124
----
commit 216dd08ce5113e2e14bb3a15765491755cbba6f5
Author: olafgarten <mi...@gmail.com>
Date: 2016-08-15T10:47:20Z
Added code to remove TIMESTAMP tokens from SQLServer queries
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #124: Added code to remove TIMESTAMP tokens from SQLS...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/124#discussion_r74863473
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/SQLServerQueryRewriter.java ---
@@ -116,6 +116,14 @@ public String rewriteFilterItem(FilterItem item) {
final String dateTimeValue = "CAST('" + format.format(date) + "' AS DATETIME)";
sb.append(dateTimeValue);
+
+ //Remove TIMESTAMP token as SQL Server doesn't support it
+ int timestampIndex = sb.lastIndexOf("TIMESTAMP");
--- End diff --
Hmm where does this "TIMESTAMP" get added to `sb` in the first place?? I think you may be fixing something that does not exist here?
(But I do think TIMESTAMP will be added by the default Query.toSql() and .toString() methods ... This is just for display, not the actual SQL that will be sent to the DB ... maybe you're confusing the two?)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #124: Added code to remove TIMESTAMP tokens from SQLServer q...
Posted by olafgarten <gi...@git.apache.org>.
Github user olafgarten commented on the issue:
https://github.com/apache/metamodel/pull/124
Hi, If this is all okay can it be merged as soon as possible, we have a customer deadline very soon and need this to be done as quickly as possible.
Thanks,
Harris Mirza
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #124: Added code to remove TIMESTAMP tokens from SQLServer q...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/124
Hi Harris,
We will do it as quickly as possible, but not quicker than allowing us to vet the change properly :-) And I would guess that you also would need us to release the fix soon then? Speaking from experience, you might want to mitigate that dependency risk a bit. If the deadline is soon I suggest you to temporarily depend on your own fork of the project here on GitHub. You should be able to make modifications and do an internal release or something like that - depending on how close and important that deadline is of course.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #124: Added code to remove TIMESTAMP tokens from SQLS...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/124#discussion_r75277252
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/SQLServerQueryRewriter.java ---
@@ -116,6 +116,14 @@ public String rewriteFilterItem(FilterItem item) {
final String dateTimeValue = "CAST('" + format.format(date) + "' AS DATETIME)";
sb.append(dateTimeValue);
+
+ //Remove TIMESTAMP token as SQL Server doesn't support it
+ int timestampIndex = sb.lastIndexOf("TIMESTAMP");
--- End diff --
Hmmm, weird. I guess it could happen in other cases, then. My testing also seemed to show that the SqlServerQueryRewriter more or less deferred to the DefaultQueryRewriter directly after looking at the types etc..
@olafgarten I'm afraid you'll have to create a unit test showing the problem before we can act on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #124: Added code to remove TIMESTAMP tokens from SQLS...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/124#discussion_r75043572
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/SQLServerQueryRewriter.java ---
@@ -116,6 +116,14 @@ public String rewriteFilterItem(FilterItem item) {
final String dateTimeValue = "CAST('" + format.format(date) + "' AS DATETIME)";
sb.append(dateTimeValue);
+
+ //Remove TIMESTAMP token as SQL Server doesn't support it
+ int timestampIndex = sb.lastIndexOf("TIMESTAMP");
--- End diff --
But that info would then also suggest that this fix is wrong because a few lines up (line 105) it says `if (operand instanceof Date) {`and I guess that is NOT the case when the operand is a string.
The point of this PR should at least be proven with a unittest because right now I think it doesn't actually fix the problem, and it also does not prove that a problem exist (I believe that it exist, I just don't think it is in these particular lines of code).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #124: Added code to remove TIMESTAMP tokens from SQLS...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/124#discussion_r74890818
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/SQLServerQueryRewriter.java ---
@@ -116,6 +116,14 @@ public String rewriteFilterItem(FilterItem item) {
final String dateTimeValue = "CAST('" + format.format(date) + "' AS DATETIME)";
sb.append(dateTimeValue);
+
+ //Remove TIMESTAMP token as SQL Server doesn't support it
+ int timestampIndex = sb.lastIndexOf("TIMESTAMP");
--- End diff --
It _is_ using `FilterItem.toSql()` directly when the operator is a string. It ends up in `AbstractQueryRewriter.rewriteFilterItem()` line 230 (in current master), which looks like this:
```Java
final String primaryFilterSql = item.toSql(isSchemaIncludedInColumnPaths());
```
If using `java.sql.Date` or `java.util.Date` instead of a string, everything works fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #124: Added code to remove TIMESTAMP tokens from SQLServer q...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/124
I vote to close this, unless anyone would like to implement the needed tests and verifications that where asked for in review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #124: Added code to remove TIMESTAMP tokens from SQLS...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/124
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #124: Added code to remove TIMESTAMP tokens from SQLS...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/124#discussion_r74885525
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/dialects/SQLServerQueryRewriter.java ---
@@ -116,6 +116,14 @@ public String rewriteFilterItem(FilterItem item) {
final String dateTimeValue = "CAST('" + format.format(date) + "' AS DATETIME)";
sb.append(dateTimeValue);
+
+ //Remove TIMESTAMP token as SQL Server doesn't support it
+ int timestampIndex = sb.lastIndexOf("TIMESTAMP");
--- End diff --
I can confirm that a TIMESTAMP is added when using this kind of query:
```Java
final Table dateTest = strategy.getDefaultSchema().getTableByName("datetest");
final Column created = dateTest.getColumnByName("created");
final Column name = dateTest.getColumnByName("name");
Query q = new Query().select(name).from(dateTest).where(created, OperatorType.GREATER_THAN, "2015-01-01);
```
Retrieved from SQL Server Profiler:
```SQL
SELECT dbo."datetest"."name" FROM dbo."datetest" WHERE dbo."datetest"."created" > TIMESTAMP '2015-01-01 00:00:00'
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---