Repository: jsonata-js/jsonata (js)

Issue:
Semantic Issue in Object Grouping
I noticed, while reviewing the evaluation code, that there is what I think is an incorrect assumption.  The evaluation for objects appears to work as follows:

We loop over each input value.  We evaluate the `key` expression in the context of that input value.  We then append that input value to the list of values we will use later for that key (more on that in a second).  We *also* note the expression associated with that key and record that as well so we can evaluate that value expression in the context of each of the inputs we are storing away.  But here is the problem...the value expression isn't always the same but we only store the first one we run into.  I'm pretty sure this isn't correct and it creates some odd issues.  For example, imagine our input (for all cases I'm going to consider) is:

```json
[
  {
    "key": "foo",
    "value": 5
  },
  {
    "key": "bar",
    "value": 10
  }
]
```

Now if I evaluate this: `{ key: value }`, I get pretty much what you might expect, *i.e.,* `{ "foo": 5, "bar": 10 }`.

So far so good.  If I evaluate `{ key: (value+1) }`, I also get pretty much what I would expect: `{ "foo": 6, "bar": 11 }`.

Now how about if I evaluate: `{ key: value, key: value }`.  Now we have **overlapping values for the key expressions**.  That's actually ok and it works (and I think it is reasonable to allow overlapping key expressions, BTW).  It evaluates to:

```json
{
  "foo": [
    5,
    5
  ],
  "bar": [
    10,
    10
  ]
}
```

I'm no so sure this is the most intuitive result.  Some might expect the second to *overwrite* the first in this case (*i.e.*, some might expect `{ "foo": 5, "bar": 10 }`), but it doesn't.  Instead, it appends the results.  But at least we get an answer that we can explain even if it might seem unintuitive.

But now things start to misbehave.  Imagine I evaluate this instead: `{ key: value, key: value+1 }`.  I get the same answer as for `{ key: value, key: value }`.  This is because `value` is tucked away as the expression associated with `"foo"` and `"bar"` because it is the first one encountered, so `value+1` **is never evaluated**.  But things can actually get worse.  If you switch the order and try to evaluate `{ key: value+1, key: value }` you actually get an error that it cannot evaluate `value` because it isn't a number.

Now, one thing you can do is change the semantics slightly so you actually evaluate the `value` expressions immediately in the context of each input value (rather than storing them up).  This actually makes sense for the `value+1` case and seems reasonably intuitive.  Said another way, it may be more intuitive to do the evaluate this way because I suspect it is what people would expect.  But, this will actually breaks one of the test cases, *i.e.,* the containing the expression:

```Account.Order.Product{$string(ProductID): (Price)[0]}```

Because the `[0]` in there become meaningless because `Price` always evaluates to a scalar rather than an array.  So that approach would change the semantics slightly.  Presumably the `[0]` is there specifically to grab the first value.

The alternative is to append not just each successive input value associated with each key but also each unique value expression as well (instead of just the first one).  I suspect you'd probably want this alternative but I wanted to thoroughly explain the problem and collect feedback to be sure.

To be honest, implementing these evaluation semantics is a bit complicated.  Furthermore, that error for `value+1` is pretty confusing and unintuitive.  So before working on implementing anything more (I've implemented a couple different approaches already), I thought it would be good to discuss what is most intuitive.

Any comments?

Pull request description:
throw error if multiple group-by expressions evaluate to same key
New dynamic error created which is thrown if multiple clauses within a 'group-by' expression `{...}` evaluate to the same key.  Previously, unexpected results would get returned.

Resolves #163 as agreed with @xogeny 

Patch:
diff --git a/jsonata.js b/jsonata.js
index 8c33752..c3d0c61 100644
--- a/jsonata.js
+++ b/jsonata.js
@@ -2205,6 +2205,7 @@ var jsonata = (function() {
         if (!Array.isArray(input)) {
             input = createSequence(input);
         }
+
         for(var itemIndex = 0; itemIndex < input.length; itemIndex++) {
             var item = input[itemIndex];
             for(var pairIndex = 0; pairIndex < expr.lhs.length; pairIndex++) {
@@ -2219,9 +2220,20 @@ var jsonata = (function() {
                         value: key
                     };
                 }
-                var entry = {data: item, expr: pair[1]};
+                var entry = {data: item, exprIndex: pairIndex};
                 if (groups.hasOwnProperty(key)) {
                     // a value already exists in this slot
+                    if(groups[key].exprIndex !== pairIndex) {
+                        // this key has been generated by another expression in this group
+                        // when multiple key expressions evaluate to the same key, then error D1009 must be thrown
+                        throw {
+                            code: "D1009",
+                            stack: (new Error()).stack,
+                            position: expr.position,
+                            value: key
+                        };
+                    }
+
                     // append it as an array
                     groups[key].data = functionAppend(groups[key].data, item);
                 } else {
@@ -2233,7 +2245,7 @@ var jsonata = (function() {
         // iterate over the groups to evaluate the 'value' expression
         for (key in groups) {
             entry = groups[key];
-            var value = yield * evaluate(entry.expr, entry.data, environment);
+            var value = yield * evaluate(expr.lhs[entry.exprIndex][1], entry.data, environment);
             if(typeof value !== 'undefined') {
                 result[key] = value;
             }
@@ -4814,6 +4826,16 @@ var jsonata = (function() {
     /**
      * Error codes
      *
+     * Sxxxx    - Static errors (compile time)
+     * Txxxx    - Type errors
+     * Dxxxx    - Dynamic errors (evaluate time)
+     *  01xx    - tokenizer
+     *  02xx    - parser
+     *  03xx    - regex parser
+     *  04xx    - function signature parser/evaluator
+     *  10xx    - evaluator
+     *  20xx    - operators
+     *  3xxx    - functions (blocks of 10 for each function)
      */
     var errorCodes = {
         "S0101": "String literal must be terminated by a matching quote",
@@ -4850,6 +4872,7 @@ var jsonata = (function() {
         "T1006": "Attempted to invoke a non-function",
         "T1007": "Attempted to partially apply a non-function. Did you mean ${{{token}}}?",
         "T1008": "Attempted to partially apply a non-function",
+        "D1009": "Multiple key definitions evaluate to same key: {{value}}",
         "T2001": "The left side of the {{token}} operator must evaluate to a number",
         "T2002": "The right side of the {{token}} operator must evaluate to a number",
         "T2003": "The left side of the range operator (..) must evaluate to an integer",


Test patch:
diff --git a/test/test-suite/groups/object-constructor/case017.json b/test/test-suite/groups/object-constructor/case017.json
index a0cf596..730fcc5 100644
--- a/test/test-suite/groups/object-constructor/case017.json
+++ b/test/test-suite/groups/object-constructor/case017.json
@@ -2,20 +2,5 @@
     "expr": "Account.Order.Product{$.\"Product Name\": Price, $.\"Product Name\": Price}",
     "dataset": "dataset5",
     "bindings": {},
-    "result": {
-        "Bowler Hat": [
-            34.45,
-            34.45,
-            34.45,
-            34.45
-        ],
-        "Trilby hat": [
-            21.67,
-            21.67
-        ],
-        "Cloak": [
-            107.99,
-            107.99
-        ]
-    }
+    "code": "D1009"
 }
\ No newline at end of file
diff --git a/test/test-suite/groups/object-constructor/case018.json b/test/test-suite/groups/object-constructor/case018.json
index 69086d8..cec8053 100644
--- a/test/test-suite/groups/object-constructor/case018.json
+++ b/test/test-suite/groups/object-constructor/case018.json
@@ -1,21 +1,17 @@
 {
-    "expr": "Account.Order.Product{`Product Name`: Price, `Product Name`: Price}",
+    "expr": "Account.Order.Product{`Product Name`: Price, SKU: Price}",
     "dataset": "dataset5",
     "bindings": {},
     "result": {
         "Bowler Hat": [
-            34.45,
-            34.45,
             34.45,
             34.45
         ],
-        "Trilby hat": [
-            21.67,
-            21.67
-        ],
-        "Cloak": [
-            107.99,
-            107.99
-        ]
+        "0406654608": 34.45,
+        "Trilby hat": 21.67,
+        "0406634348": 21.67,
+        "040657863": 34.45,
+        "Cloak": 107.99,
+        "0406654603": 107.99
     }
 }
\ No newline at end of file
diff --git a/test/test-suite/groups/object-constructor/case023.json b/test/test-suite/groups/object-constructor/case023.json
new file mode 100644
index 0000000..1224945
--- /dev/null
+++ b/test/test-suite/groups/object-constructor/case023.json
@@ -0,0 +1,11 @@
+{
+    "expr": "{ type: $average(value), kind: $sum(value) } ",
+    "data": [
+        { "type": "a", "kind": "a", "value": 0 },
+        { "type": "a", "kind": "b", "value": 1 },
+        { "type": "b", "kind": "a", "value": 2 },
+        { "type": "b", "kind": "b", "value": 3 }
+    ],
+    "bindings": {},
+    "code": "D1009"
+}
\ No newline at end of file