You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by narahari92 <gi...@git.apache.org> on 2015/03/30 18:08:03 UTC
[GitHub] metamodel pull request: Fix for METAMODEL-128
GitHub user narahari92 opened a pull request:
https://github.com/apache/metamodel/pull/13
Fix for METAMODEL-128
Please review the code fix for the mentioned defect. One test case gives false failure because we are comparing query's string form when an alias of item is used. I have modified that.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/narahari92/metamodel master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/13.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 #13
----
commit 4b7ade730734038503d29c5157fb842934f511a3
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-25T15:59:14Z
changing select(expression) method of Query to support multiple columns and expressions in single string
commit a6684b9dde12e7766612a58940ab951bcf4f48c9
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-26T14:53:20Z
change to select(expression) method of Query to have multiple expression in single method
commit 3436ef2bb487311ea563e0bab5e8b5986ec52327
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-26T14:57:31Z
Unit test for issue METAMODEL-126
commit 6b13a57033c5e84f7f635c858d9531992e8b2780
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-28T15:38:53Z
corrected the exprected and actual parameters location in method signature
commit 724bdc7a3a70fb70112b05165ee389d0b13d573a
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-29T05:20:07Z
refactored select(expression) method of Query
commit 9238e8c4947385db8c046ef1e86cf6d90acf2ab2
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-29T13:24:03Z
merging
commit a46b432a5fa0b326ab400e4c3510b61214152d79
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-30T15:20:51Z
"Regular merge"Merge remote-tracking branch 'upstream/master'
commit 04f36b081cf9a657fb9462f9e9a87bb0c27ca284
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-30T15:42:36Z
Fix for defect METAMODEL-128
commit 9b8f9f7d2d38bf8bdcdd7b2d15bee89d494c27a6
Author: Hosur Narahari <hn...@gmail.com>
Date: 2015-03-30T15:46:31Z
Fix for defect METAMODEL-128
----
---
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: Fix for METAMODEL-128
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-87759734
Yes normally I would also be in favor of making a copy. But in this case it is really a "reference" and not an independent item. So if you change that then I think we can make the general change a lot less - then you don't need to make the clone method public and you don't need to change the existing unittest.
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-87761146
No that existing unit test still changes because the alias name in query will be replaced by <tablename>.<columnName>.i.e. "foobarbaz" gets replaced by "a.foo"
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-87758643
I thought returning the same reference might prove risky and have unexpected behavior. But yes, what you said makes sense.
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-87767075
Hi, I have made the change to return same item instead of clone.
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88341746
We can also use subqueries in where clause. I successfully executed below query.
Query query = context.parseQuery("select username from AMASS_USER where password in (select password from AMASS_USER where enabled=1)");
Considering all the places where we can have alias name for columns wouldn't it be better to have the getAllSelectItems() method. Or shall I still remove the method and use only items from select clause.
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88024935
SELECT... FROM (SELECT name as n FROM ...) as q where n='foo'; is perfectly valid but will fail if we don't consider select items of other clauses if and when we support sub queries for from and where clause in MetaModel.
---
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: Fix for METAMODEL-128
Posted by ashish-m-yh <gi...@git.apache.org>.
Github user ashish-m-yh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/13#discussion_r27453436
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -160,6 +164,14 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
return new SelectItem(subQuerySelectItem, fromItem);
}
}
+
+ //if the expression is alias of some select item defined return clone of that select item
+ List<SelectItem> allSelectItems = MetaModelHelper.getAllSelectItems(_query);
--- End diff --
Isn't a SELECT in WHERE clause possible in case of sub-select?
---
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: Fix for METAMODEL-128
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/13
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88528119
Committed with recommended changes...
---
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: Fix for METAMODEL-128
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88384811
I believe that where clause will only work on JDBC because it gets carried along as one big expression. But MM unfortunately will not understand it so no select items will be there anyway. Hmm how about then making a getSelectItemByAlias(...) method. We could start with just items from SELECT but could eventually expand it if subquery support increases.
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88172276
Hey, I noticed one thing just now. We can actually use subqueries in from clause at least for JdbcDataContext. Below is the query I ran which was successful.
Query query = dataContext.parse("SELECT password as X from (select username,password from AMASS_USER) S");
---
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: Fix for METAMODEL-128
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/13#discussion_r27455199
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -160,6 +164,14 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
return new SelectItem(subQuerySelectItem, fromItem);
}
}
+
+ //if the expression is alias of some select item defined return clone of that select item
+ List<SelectItem> allSelectItems = MetaModelHelper.getAllSelectItems(_query);
--- End diff --
Well actually MetaModel's query API still does not support sub-select, so no. But even if it did, would it make sense to have an alias in the subquery that is then referred to in the outer query? At least then it would also be qualified I think, a la:
SELECT ..., (SELECT name as n FROM ...) AS q FROM ... WHERE q.n = 'foo'
But to summarize: I don't think this is a scenario we need to cover at all since only regular subqueries (in the FROM clause) is supported in MetaModel at the moment.
---
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: Fix for METAMODEL-128
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/13#discussion_r27423309
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -160,6 +164,14 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
return new SelectItem(subQuerySelectItem, fromItem);
}
}
+
+ //if the expression is alias of some select item defined return clone of that select item
+ List<SelectItem> allSelectItems = MetaModelHelper.getAllSelectItems(_query);
--- End diff --
Also reflecting a bit on this ... Do we need to have this getAllSelectItems(...) method?
What I mean is, could there be aliased select items anywhere else than in the SELECT clause? I cannot imagine it really ... For instance in WHERE clause I have never seen anything a la:
SELECT ... FROM ... WHERE name AS n = "Kasper"
If we assume that this never happens then you can get rid of the getAllSelectItems(...) method which is already a bit greedy (makes new lists etc.). If I am wrong about the assumption (I seriously am in doubt) then I think the solution is good!
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-87761276
Anyhow I'll make that change you suggested
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88022092
Fine I'll remove that getAllSelectItems() method but isn't subquery in from and where class there in to do list of MetaModel. I remember seeing it in jira for improvement.
---
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: Fix for METAMODEL-128
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/13#discussion_r27410445
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -160,6 +164,14 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
return new SelectItem(subQuerySelectItem, fromItem);
}
}
+
+ //if the expression is alias of some select item defined return clone of that select item
+ List<SelectItem> allSelectItems = MetaModelHelper.getAllSelectItems(_query);
+ for(SelectItem selectItem : allSelectItems) {
+ if(selectItem.getAlias() != null && selectItem.getAlias().equals(expressionCopy)) {
+ return selectItem.clone();
--- End diff --
Do we need to return the clone? Why not return the same select item? If it is referred to by alias (like a kind of pass by reference) then I think it makes sense to keep the same reference item and not a copy.
---
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: Fix for METAMODEL-128
Posted by ashish-m-yh <gi...@git.apache.org>.
Github user ashish-m-yh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/13#discussion_r27457191
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -160,6 +164,14 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
return new SelectItem(subQuerySelectItem, fromItem);
}
}
+
+ //if the expression is alias of some select item defined return clone of that select item
+ List<SelectItem> allSelectItems = MetaModelHelper.getAllSelectItems(_query);
--- End diff --
Sounds good then.
---
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: Fix for METAMODEL-128
Posted by narahari92 <gi...@git.apache.org>.
Github user narahari92 commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88439981
Ya.. that sounds like a good plan.. I'll make the necessary changes and commit it today
---
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: Fix for METAMODEL-128
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/13#issuecomment-88277332
Correct, that's the one type of subquery we support. One that is in the FROM clause. We don't support it in select or where (currently).
---
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.
---