Implement document expressions

[katcharov]

JAVA-4782

[jyemin]

This PR isn't in a reviewable state (only the last commit seems relevant).

[katcharov]

@jyemin When I rebase one of the branches, all of the ensuing branches need to be rebased (with conflicts resolved along the way). Sometimes I'm caught in the middle of this process (this happened to @stIncMale a couple of times), and the diff is unreadable. This has been resolved in array, date, and conv, and soon in this branch.

[stIncMale]

We don't have Expression.of(Number) in the public API, but we have getNumber(..., Number orElse). It appears that we should either not have getNumber(..., Number orElse), or have it and then have Expression.of(Number) in the public API. And if we have Expression.of(Number), we don't need Expression.of(double)/Expression.of(Decimal128).

[katcharov]

The of methods return their type. But these have a type after get, and need to return that type. This is confusing to users if we don't use Number, unlike the case with of. If you still perceive an inconsistency, let's add a note to the final doc to consider switching of to use Number.

[stIncMale]

The of methods return their type. But these have a type after get, and need to return that type. This is confusing to users if we don't use Number, unlike the case with of.

I think, this argument confuses the MQL number type (expressed in Java as NumberExpression and mentioned in the getNumber method name) with the Java Number (specified as the type of the second parameter) - they are different types. Also, if this argument were applied consistently, then neither DocumentExpression.getInteger(String, long) nor Expressions.ofNumberArray(double...)/(Decimal128...) should not have been introduced as they violate the approach implied by the argument.

If you still perceive an inconsistency, let's add a note to the final doc to consider switching of to use Number.

The API inconsistency was introduced in this PR, and deciding what to do with it does not require postponing as far as I can see (if there were specific future decisions you foresee on which this decision has to rely on, I would expect those to have been already mentioned).

---

The current integer-specific API is consistent between Expressions and DocumentExpression:

IntegerExpression Expressions.of(int)
IntegerExpression Expressions.of(long)

IntegerExpression DocumentExpression.getInteger(String, int)
IntegerExpression DocumentExpression.getInteger(String, long)

the API that is not integer-specific is not consistent between Expressions and DocumentExpression:

NumberExpression Expressions.of(double)
NumberExpression Expressions.of(Decimal128)

NumberExpression DocumentExpression.getNumber(String, Number)

To achieve consistency we should

1. either replace getNumber(String, Number) with getNumber(String, double) and getNumber(String, Decimal128);
2. or replace of(double) and of(Decimal128) with of(Number) (in this case we also need to replace Expressions.ofNumberArray(double...)/(Decimal128...) with Expressions.ofNumberArray(Number...)).

Second option reduces the number of overloads at the cost of shifting some type-checks from compile-time to run-time. I will be fine with either option.

[katcharov]

I think what you are saying is correct. I will consider the two options.

[katcharov]

I chose the first one, because it is more consistent with the nearby getInteger methods.

[stIncMale]

The of methods return their type. But these have a type after get, and need to return that type. This is confusing to users if we don't use Number, unlike the case with of.

I think, this argument confuses the MQL number type (expressed in Java as NumberExpression and mentioned in the getNumber method name) with the Java Number (specified as the type of the second parameter) - they are different types. Also, if this argument were applied consistently, then neither DocumentExpression.getInteger(String, long) nor Expressions.ofNumberArray(double...)/(Decimal128...) should not have been introduced as they violate the approach implied by the argument.

If you still perceive an inconsistency, let's add a note to the final doc to consider switching of to use Number.

The API inconsistency was introduced in this PR, and deciding what to do with it does not require postponing as far as I can see (if there were specific future decisions you foresee on which this decision has to rely on, I would expect those to have been already mentioned).

---

The current integer-specific API is consistent between Expressions and DocumentExpression:

IntegerExpression Expressions.of(int)
IntegerExpression Expressions.of(long)

IntegerExpression DocumentExpression.getInteger(String, int)
IntegerExpression DocumentExpression.getInteger(String, long)

the API that is not integer-specific is not consistent between Expressions and DocumentExpression:

NumberExpression Expressions.of(double)
NumberExpression Expressions.of(Decimal128)

NumberExpression DocumentExpression.getNumber(String, Number)

To achieve consistency we should

1. either replace getNumber(String, Number) with getNumber(String, double) and getNumber(String, Decimal128);
2. or replace of(double) and of(Decimal128) with of(Number) (in this case we also need to replace Expressions.ofNumberArray(double...)/(Decimal128...) with Expressions.ofNumberArray(Number...)).

Second option reduces the number of overloads at the cost of shifting some type-checks from compile-time to run-time. I will be fine with either option.

[stIncMale]

Sorry, I have to press "submit" even though I have not finished re-reviewing because GitHub UI messed up the display of pending comments, and I am afraid I may lose them.

[jyemin]

What is the behavior of doc.getBoolean("a.b"). Does it

1. return the value of the "a.b" field in this document
2. return the value of the "b" field inside the "a" field (assuming the value of the "a" field is a document)

[jyemin]

If 1., the parameter should be named field. If 2. is there any way to use dotted path notation in this API, and if not, should there be?

[jyemin]

If we're not supporting dotted path notation, I worry that because their use is so prevalent in the wild, applications will make the mistake of using them and being really confused by the results they get back

[katcharov]

The intent was to return (1) the value of the "a.b" field, not the path.

The potential error you mention seems serious to me, but I am unsure of how to address it.

• It seems that there needs to be some way to use a literal field notation, so whatever we do we will need one Expression get() or many getters that take a proper field.
• We could try getBooleanField and getBooleanPath, but this seems like overkill, and perhaps would not address the possible user error.
• There is a get(key) on MapExpression that does not and should not take a path. MqlExpression implements both interfaces.
• If we can implement what I have been calling "schema objects" (perhaps "object bindings"?) then one can write: doc.a.b. I think this would disincline users from any of the getField methods, and avoid the issue, but only to the extent that object bindings are possible and used by users.

I'll keep thinking. Suggestions welcome.

[jyemin]

I think the more common case will be people wanting to treat the field names as paths that traverse subdocuments. It's only in the last couple of years that the server or drivers even allowed field names with dots in them. So that suggests that the default behavior should be to treat dotted field names as paths, and have an explicit way to treat them as field names.

At the same time, it's an attractive option to use this opportunity to "fix" the behavior, and provide explicit syntax for dotted paths. So something like:

   Expression getField(String fieldName);    // treat "a.b" as a field name
   Expression getField(List<String> path);   // treat each string in the list as a field name

[stIncMale]

At the same time, it's an attractive option to use this opportunity to "fix" the behavior, and provide explicit syntax for dotted paths. So something like:
getField(List path)

If we use any type but String to represent field names / paths, I think we should introduce a special type instead of using List<String>.

I would also prefer to just introduce a Path type with two ways of creating its instances, something like Path.name and Path.path (in the search API we have SearchPath.fieldPath and SearchPath.wildcardPath, and we never accept plain String as a path in the rest of the API). We may even introduce two subtypes if there are situations in MQL when a path is not allowed and only a field name may be used (I don't know if there are such situations). I discussed this with Maxim, and he does not like the idea due to its verbosity, others may think the same. But I think that introducing a type that covers both field names (just a degenerate case of a field path) and field paths is the right way to approach the task: a plain String can technically be used to represent a full name of a human, text of a book, a path in a file system, a field path in MongoDB, but it does not mean that using a plain String for any of that is necessarily a good idea. Having a special type allows one to both express the meaning of a value and validate it at creation.

[katcharov]

Discussed, all prefer the getInteger(String fieldName). It would be beneficial to have a way to query by paths, but that can be considered later.

[katcharov]

Added tests. "fieldName" is a fairly long parameter hint in IntelliJ, but let's see how that goes.

[jyemin]

Please add tests for field names with embedded . characters so that we have regression tests on the behavior

[katcharov]

Also need to check if this is broken in the current implementation - on the first reading of the getField docs, it seemed it would be automatically treated as a literal, but I think it needs to be explicitly wrapped.

[jyemin]

Is this done?

[katcharov]

Yes

[jyemin]

Both setField and getField allow the field name itself to be an expression. That API should support that as well.

[katcharov]

This is supported for Map (arbitrary number of string keys to values all of the same type), but is not supported for Documents, which represent records (fixed number of string-named fields to values of different types). Neither one extends the other, because:

1. A record cannot have a set(T) method since this would allow fields to be set via a broader type than they actually are.
2. A map<StringEx> cannot have a method getBoolean(field) because this would allow string values to be pulled out as booleans.

Any situation that requires this is almost certainly more correctly handled via a MapExpression<Expression>, rather than a document (record).

[jyemin]

What is a MapExpression? I don't see that anywhere.

[katcharov]

It's in #1054

[jyemin]

Can you show me what the code would look like? I don't understand how it works in practice, e.g. say I have a document like:

{
   s1 : "str1",
   s2: "str2",
   s3 : "str3",
   i: 1
}

How would I do something like set one of the s fields where the suffix of the field name is the value of the i field?

[katcharov]

This should work:

MapExpression<Expression> map = ofMap(<above map here>);
StringExpression key = of("s").concat(map.get("i").asString());
map.set(key, of("new_value"))

This particular operation should not be done to a Document/Record, for the same reason this is not available in Java except through the use of reflection.

[jyemin]

Deferring at least to map expression PR

[stIncMale]

I propose using of(other) instead of Expressions.numberToExpression(other). When this is done, please consider the PR approved by me.

[jyemin]

Is this done?

[katcharov]

Yes

[jyemin]

Updated some of the comment threads that I'm on.