AQL query does not properly handle null detection

[Calrain]

My Environment

• ArangoDB Version: 3.6.5
• Storage Engine: RocksDB
• Deployment Mode: Single Server
• Deployment Strategy: Manual Start
• Configuration: Running in Windows 2016 Server
• Infrastructure: own
• Operating System: Windows 2016 Server
• Total RAM in your machine: 8Gb
• Disks in use: SSD

Component, Query & Data

Affected feature:
AQL does not handle null detection with subqueries

__AQL query:

LET data = [
    {
        product: 1,
        info: [
            "advocado",
            "banana"
        ]
    },
    {
        product: 2,
        info: null
    },
    {
        product: 3,
        info: [
            "coconut",
            "mango"
        ]
    }
]

FOR d IN data
RETURN {
    product: d.product,
    summary: (d.info == null ? 'none' : CONCAT(FOR i IN d.info RETURN i))
}

When this command runs, I get this error:
Query: AQL: collection or array expected as operand to FOR loop; you provided a value of type 'null' (while executing)

I would expect this query to work.

__AQL explain:

Execution plan:
 Id   NodeType            Est.   Comment
  1   SingletonNode          1   * ROOT
  2   CalculationNode        1     - LET data = [ { "product" : 1, "info" : [ "advocado", "banana" ] }, { "product" : 2, "info" : null }, { "product" : 3, "info" : [ "coconut", "mango" ] } ]   /* json expression */   /* const assignment */
  4   EnumerateListNode      3     - FOR d IN data   /* list iteration */
 12   SubqueryStartNode      3       - LET #3 = ( /* subquery begin */
  6   CalculationNode        3         - LET #7 = d.`info`   /* attribute expression */
  7   EnumerateListNode    300         - FOR i IN #7   /* list iteration */
 13   SubqueryEndNode      300       - RETURN  i ) /* subquery end */
 10   CalculationNode      300       - LET #9 = { "product" : d.`product`, "summary" : ((d.`info` == null) ? "none" : CONCAT(#3)) }   /* simple expression */
 11   ReturnNode           300       - RETURN #9

Indexes used:
 none

Functions used:
 Name     Deterministic   Cacheable   Uses V8
 CONCAT   true            true        false  

Optimization rules applied:
 Id   RuleName
  1   move-calculations-up
  2   remove-redundant-calculations
  3   remove-unnecessary-calculations
  4   splice-subqueries

Dataset:
This query has the dataset included, but I've found this exact problem with many queries that I've had to work with.

When running a query over an array of objects, and where you're trying to look at a key in those objects that is also an array, it seems to be impossible to get AQL to detect that it's a null valued key.

What happens is that subqueries that are only supposed to run on the key if it's non-null still fire off.

In the example above you can see (apologies for the formatting, just helps me understand it) that I am trying to determine if d.info is null, because if it is I want to return a simple string called 'none', but only if it's non-null should it perform that CONCAT command that iterates over d.info.

It seems the AQL is trying to still run over d.info, so I'm not sure if it's a bug with my query, a bad way of detecting if d.info is null, or something else that I'm missing.

Steps to reproduce

1. Run the query above, that should show it

Problem:
Trying to get this AQL query to work.

I know that a simple solution is to ensure the second product have an .info key of [] instead of being null, but there are real situations where it's not possible to do that.

This example is a simplified version of it happening with much more complicated queries with COLLECT and AGGREGATE function use. Interestingly those commands aren't part of the problem, it seems to be that I'm just unable to get AQL to not perform that CONCAT (or any other query that iterates over d.info) if d.info is null.

Expected result:

[
  {
    "product": 1,
    "summary": "advocadobanana"
  },
  {
    "product": 2,
    "summary": "none"
  },
  {
    "product": 3,
    "summary": "coconutmango"
  }
]

Note: Flipping around the ternary operator doesn't fix it either, e.g. using:

FOR d IN data
RETURN {
    product: d.product,
    summary: (d.info ? CONCAT(FOR i IN d.info RETURN i) : 'none')
}

[jsteemann]

The issue here is not directly related to null detection, but rather the subquery used inside an expression where it is supposed to run only conditionally.
Here is the problematic part:

(d.info == null ? 'none' : CONCAT(FOR i IN d.info RETURN i))

This (internally) gets executed as

LET temp = CONCAT(FOR i IN d.info RETURN i)
...
(d.info == null ? 'none' : temp)

meaning the subquery is pulled out of the expression and executed before it unconditionally.
That's why in this query it will fail for any info attributes that are no arrays.

This is a known limitation and has been brought up several times, because it is unintuitive behavior and contrary to how all other operands work in AQL expressions.

In this particular query the problem is easy to circumvent by not using a subquery:

FOR d IN data
RETURN {
    product: d.product,
    summary: (d.info ? CONCAT(d.info[*]) : 'none')
}

In other queries it may not be as easy
Ideally, the optimizer could rewrite expressions with subqueries so that they are executing only conditionally. But this is substantial work and hasn't been addressed yet.

[Calrain]

Thanks for that @jsteemann

In my use case it was caught in a ~200 line AQL query that aggregated data and merged data, so I had to rewrite the flow to ensure an empty array was returned instead of a null key.

I can see why it happens, and hopefully there will be time in the future to fix this edge case :)

Maybe if the rewriter detected ternary operators, and then replicated the operator?

LET temp = (d.info == null ? 'none' : CONCAT(FOR i IN d.info RETURN i))
...
(d.info == null ? 'none' : temp)

It's OK to close this issue, was hoping I was doing something wrong.

Cheers

[jsteemann]

That solution would make it work for ternary operators, but unfortunately subqueries can used in lots of places, not just for ternary operators. Here is a (very) contrived example:

1+ LENGTH(subquery1) * (
  LENGTH(subquery2) > 25 ? 
    LENGTH(subquery3) + LENGTH(subquery4) : ( 
      LENGTH(subquery5) > 0 ? 
        LENGTH(subquery6) + LENGTH(subquery7) :
        LENGTH(subquery8) + LENGTH(subquery9)
    )
) 

That's what makes the rewriting kind of complicated.

I am happy to leave this issue open as a reminder for us to address this.
Even though this is a known limitation, it is non-intuitive behavior and in contrast to what users expect. So I consider this a bug.

[Calrain]

Yes, I can see that.

To me it looks like AQL is handling the error too early, and not letting the ternary operator protect the query.

For example:
When subqueries are pulled out, rather than pulling it out exactly as it is, couldn't you put it in a try/catch style wrapper, and if it throws an AQL error while executing, then assign that AQL error the variable but don't cancel execution of the query.

Then, later if anything tries to read the value of that pulled out expression, only then will the AQL error be thrown.

For example it could become:

LET temp = TRY(CONCAT(FOR i IN d.info RETURN i))
...
(d.info == null ? 'none' : temp)

With this, if an error happens in the TRY( ) function, then it doesn't break the AQL command, it just assigns the AQL error object to the 'temp' variable.

If the user has properly constructed their query, their ternary operator will stop the value of 'temp' being read, and the query will execute as expected.

In your contrived example, it would still work, as the user has to ensure that those values aren't used, and if they are then they get the normal error.

It also may make a query run faster, because if the logic inside the TRY(..) was expensive from a processing point of view, it would probably crash early and not execute, but then again, it shouldn't have executed so the overall query will still be OK.

[Simran-B]

Another workaround:

summary: (d.info == null ? 'none' : CONCAT(FOR i IN NOT_NULL(d.info, []) RETURN i))

For the example query, you could even get away with this non-subquery solution:

summary: CONCAT(d.info) || "none"