You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by kaspersorensen <gi...@git.apache.org> on 2015/11/02 22:59:21 UTC
[GitHub] metamodel pull request: METAMODEL-195 - a MAP_VALUE function
GitHub user kaspersorensen opened a pull request:
https://github.com/apache/metamodel/pull/70
METAMODEL-195 - a MAP_VALUE function
This PR attempts to fix METAMODEL-195, the addition of a MAP_VALUE function which takes a string parameter to tell which value from within a map you wish to get. It's syntax is like this: ```MAP_VALUE('path.to.value', column)```
There is also support for a query parser shorthand syntax where you select like this: ```column.path.to.value```
It only works on MAP type columns.
Point to especially consider: Since this is the first function which involves parameters, I ended up also doing some redesign of the ScalarFunction interface. I initially built it so that parameters was part of the MapValueFunction instance and thus you would need to instantiate the function for each parameter set you have. This worked kinda well just until I came to the point of parsing and printing the function as SQL. The trouble here was that if the interface did not reveal the parameters in any way then automating the printing etc. wasn't possible either. I am a bit unhappy about changing the interface already since it was launched in MetaModel 4.4.0, but figure we kind of need it if we want to support functions like this. I actually think that the interface I ended up with here could be further improved, but this was the first stab to get your feedback.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kaspersorensen/metamodel METAMODEL-195/map-value-function
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/70.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 #70
----
commit 6bfbfa1d82b573d007193593219d6eb6ec38f466
Author: Kasper Sørensen <i....@gmail.com>
Date: 2015-10-22T19:33:26Z
METAMODEL-195: Added MAP_VALUE function
commit cb82939b6892a480f4e537277c081bb052aee6dc
Author: Kasper Sørensen <i....@gmail.com>
Date: 2015-10-23T15:39:23Z
METAMODEL-195: MAP_VALUE function special short-hand syntax
commit 8f603edfcea419fdf5a6965b3a7f14f4bee61cfa
Author: Kasper Sørensen <i....@gmail.com>
Date: 2015-11-02T21:47:34Z
METAMODEL-195: Implemented parsing and keeping of params in select item
commit 04c00e67192ffd4238cf5250e358b18934bca6f7
Author: Kasper Sørensen <i....@gmail.com>
Date: 2015-11-02T21:52:36Z
Merge branch 'master' into METAMODEL-195/map-value-function
Conflicts:
core/src/main/java/org/apache/metamodel/query/SelectItem.java
core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java
core/src/test/java/org/apache/metamodel/query/parser/QueryParserTest.java
----
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-160064824
OK I'll try and finish this one up then. I have let it linger for too long, but finally I think actually it's a pretty good change.
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-163160516
LGTM
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/70
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r45542637
--- Diff: core/src/main/java/org/apache/metamodel/query/FromClause.java ---
@@ -75,7 +75,13 @@ public FromItem getItemByReference(String reference) {
return null;
}
- private FromItem getItemByReference(FromItem item, String reference) {
+ private FromItem getItemByReference(final FromItem item, final String reference) {
+ if (reference.equals(item.toStringNoAlias(false))) {
+ return item;
+ }
+ if (reference.equals(item.toStringNoAlias(true))) {
+ return item;
+ }
final String alias = item.getAlias();
--- End diff --
Makes no difference to me, so I can easily change 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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r45444580
--- Diff: core/src/main/java/org/apache/metamodel/query/FromClause.java ---
@@ -75,7 +75,13 @@ public FromItem getItemByReference(String reference) {
return null;
}
- private FromItem getItemByReference(FromItem item, String reference) {
+ private FromItem getItemByReference(final FromItem item, final String reference) {
+ if (reference.equals(item.toStringNoAlias(false))) {
+ return item;
+ }
+ if (reference.equals(item.toStringNoAlias(true))) {
+ return item;
+ }
final String alias = item.getAlias();
--- End diff --
Wouldn't it be better to write just one if instead of two? I'd rewrite it to:
if (reference.equals(item.toStringNoAlias(false)) || reference.equals(item.toStringNoAlias(true))) {
return item;
}
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-160101871
OK, done with this PR for now. Please give another review if you can and then we can merge hopefully?
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r47067388
--- Diff: core/src/main/java/org/apache/metamodel/query/SelectItem.java ---
@@ -152,7 +166,24 @@ public SelectItem(Column column, FromItem fromItem) {
* @param fromItem
*/
public SelectItem(FunctionType function, Column column, FromItem fromItem) {
- this(column, fromItem, function, null, null, null, false);
+ this(column, fromItem, function, null, null, null, null, false);
--- End diff --
Sorry about that 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: METAMODEL-195 - a MAP_VALUE function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-158598709
I stopped a bit because it involved an interface change to ScalaFunction which I think is kind of a pity - we just launched that interface. But otoh it's maybe better to do the change early before anyone starts relying too much 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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-163144937
@kaspersorensen some minor comments take a look at them please
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-158598865
To you example JSON: right now it would only return the first. If we wanted it to be capable of changing the record count of the resulting dataset then I guess it's another pretty big change.
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-162528258
What do you think about merging this @albertostratio?
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r47067001
--- Diff: core/src/main/java/org/apache/metamodel/query/SelectItem.java ---
@@ -152,7 +166,24 @@ public SelectItem(Column column, FromItem fromItem) {
* @param fromItem
*/
public SelectItem(FunctionType function, Column column, FromItem fromItem) {
- this(column, fromItem, function, null, null, null, false);
+ this(column, fromItem, function, null, null, null, null, false);
--- End diff --
It's not possible in Java to put anything before the call to ```this(...)``` so that's not possible.
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-158321263
@kaspersorensen are you still working on this PR? I think this function is pretty useful.
And, I've got a question on this... Will it fetch all the "results" for the following json?:
```
{
"data": {
"results": [
{
"date": "2015-09-16 15:37:38",
"email": "dummy@gmail.com",
"password": 12345,
"source_type": "leakage"
},
{
"date": "2015-12-23 12:37:38",
"email": "dummy2@gmail.com",
"password": 78921,
"source_type": "another"
}
]
}
}
```
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r47059528
--- Diff: core/src/main/java/org/apache/metamodel/query/SelectItem.java ---
@@ -152,7 +166,24 @@ public SelectItem(Column column, FromItem fromItem) {
* @param fromItem
*/
public SelectItem(FunctionType function, Column column, FromItem fromItem) {
- this(column, fromItem, function, null, null, null, false);
+ this(column, fromItem, function, null, null, null, null, false);
+ if (column == null) {
+ throw new IllegalArgumentException("column=null");
+ }
+ }
+
+ /**
+ * Creates a SelectItem that uses a function with parameters on a column
+ * from a particular {@link FromItem}, for example
+ * MAP_VALUE('path.to.value', doc)
+ *
+ * @param function
+ * @param functionParameters
+ * @param column
+ * @param fromItem
+ */
+ public SelectItem(FunctionType function, Object[] functionParameters, Column column, FromItem fromItem) {
+ this(column, fromItem, function, functionParameters, null, null, null, false);
--- End diff --
Same comment as above
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r47059543
--- Diff: core/src/main/java/org/apache/metamodel/query/SelectItem.java ---
@@ -192,7 +223,7 @@ public SelectItem(FunctionType function, String expression, String alias) {
* the FromItem that holds the sub-query
*/
public SelectItem(SelectItem subQuerySelectItem, FromItem subQueryFromItem) {
- this(null, subQueryFromItem, null, null, subQuerySelectItem, null, false);
+ this(null, subQueryFromItem, null, null, null, subQuerySelectItem, null, false);
if (subQueryFromItem.getSubQuery() == null) {
--- End diff --
Same comment as above
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/70#issuecomment-158877708
@kaspersorensen I agree, the sooner we modify the interface the better.
Regarding my question about the json, if it requires another big change I guess it doesn't make sense to make it as part of this PR.
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r47059535
--- Diff: core/src/main/java/org/apache/metamodel/query/SelectItem.java ---
@@ -178,7 +209,7 @@ public SelectItem(String expression, String alias) {
* @param alias
*/
public SelectItem(FunctionType function, String expression, String alias) {
- this(null, null, function, expression, null, alias, false);
+ this(null, null, function, null, expression, null, alias, false);
if (expression == null) {
--- End diff --
Same comment as above
---
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: METAMODEL-195 - a MAP_VALUE function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/70#discussion_r47059516
--- Diff: core/src/main/java/org/apache/metamodel/query/SelectItem.java ---
@@ -152,7 +166,24 @@ public SelectItem(Column column, FromItem fromItem) {
* @param fromItem
*/
public SelectItem(FunctionType function, Column column, FromItem fromItem) {
- this(column, fromItem, function, null, null, null, false);
+ this(column, fromItem, function, null, null, null, null, false);
--- End diff --
Would it make more sense to put the if before the creation of the object? I mean if the column is null and we're throwing an exception, why are we instantiating the object?
---
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.
---