Skip to content

Implement array expressions #1043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Nov 29, 2022
Merged

Implement array expressions #1043

merged 5 commits into from
Nov 29, 2022

Conversation

katcharov
Copy link
Collaborator

@katcharov katcharov requested review from stIncMale, jyemin and rozza and removed request for stIncMale and jyemin November 10, 2022 21:15
@rozza rozza removed their request for review November 15, 2022 14:32
@katcharov katcharov requested a review from jyemin November 15, 2022 17:19
@katcharov katcharov force-pushed the expressions-arithmetic branch from 146a6be to 7058fe5 Compare November 17, 2022 20:44
jyemin
jyemin requested changes Nov 21, 2022
View reviewed changes
@katcharov katcharov force-pushed the expressions-array branch 2 times, most recently from dc538bb to d8d7994 Compare November 21, 2022 22:17
@katcharov katcharov requested a review from jyemin November 21, 2022 23:27
jyemin
jyemin approved these changes Nov 22, 2022
View reviewed changes
Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

stIncMale
stIncMale requested changes Nov 23, 2022
View reviewed changes
driver-core/src/main/com/mongodb/client/model/expressions/ArrayExpression.java

ArrayExpression<T> setUnion(ArrayExpression<T> set);

ArrayExpression<T> distinct();
Copy link
Member

@stIncMale stIncMale Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional]
Given that distinct is implemented via setUnion, the order of elements in the produced array is undefined, but it's not unreasonable for a user to expect that the order is preserved. Can you think of ways to express the undefined order via the method name? For example, the name set() implies that the result has neither duplicates nor a defined order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps union should exclude set in its name instead, and the docs should cover this?

The name set seems confusing because of setField, and asSet implies another type. I do not think distinct implies that items will remain in the same order, like for example removeDuplicates might.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we look at SQL, what we call setUnion is called union there. However, what is called distinct in SQL preserves the order, unlike what we call distinct here. I.e., distinct may be seen as implying order.

If you don't see a way to simply and clearly express the lack of order in the names, it's fine, neither do I.

As for setUnion vs union - given that we don't express the lack of order in the name of distinct, I am also fine with union.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that this does not preserve order even when there is only one set in the union:

        assertExpression(
                Arrays.asList(1, 2, 3),
                ofIntegerArray(3, 2, 1, 3, 3, 3).distinct(),

passes. I agree that given what you say, distinct will imply they remain in the same order for users familiar with SQL distinct. And even the streams distinct specifies that if it is ordered, it will remain ordered. But the two other points (setField is weird, implies other type) still seem compelling. Maybe some variation of "unique" instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with unique. If you leave distinct, I can also live with that.

@katcharov katcharov requested a review from stIncMale November 28, 2022 15:05
stIncMale
stIncMale reviewed Nov 28, 2022
View reviewed changes
Copy link
Member

@stIncMale stIncMale left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a comment so that stupid GitHub allowes me to finish the review despite my having left comments in existing discussions.

Base automatically changed from expressions-arithmetic to expressions November 29, 2022 15:31
@katcharov katcharov merged commit 7baa36c into expressions Nov 29, 2022
@katcharov katcharov deleted the expressions-array branch November 29, 2022 15:33
katcharov added a commit that referenced this pull request Jan 30, 2023
@katcharov
Implement array expressions (#1043)
a83c781 
JAVA-4805
katcharov added a commit that referenced this pull request Jan 31, 2023
@katcharov
Implement array expressions (#1043)
7ac3ccd 
JAVA-4805
katcharov added a commit that referenced this pull request Jan 31, 2023
@katcharov @stIncMale
Add MQL Expressions (#1032)
ff83341 
* Implement boolean expressions (#1025)

JAVA-4779

* Implement filter, map, reduce (#1031)

JAVA-4781

* Implement eq, ne, gt, gte, lt, lte (#1033)

JAVA-4784

* Implement string expressions (#1036)

JAVA-4801

* Implement arithmetic expressions (#1037)

Implement arithmetic expressions (from top 50, and others)

JAVA-4803

* Implement array expressions (#1043)

JAVA-4805

* Implement date expressions (#1045)

JAVA-4804

* Implement conversion/type expressions (#1050)

JAVA-4802

* Implement document expressions (#1052)

JAVA-4782

* Replace reduce with individual reductions (#1053)

JAVA-4814

* Implement map expressions (#1054)

JAVA-4817

* Implement switch expression (#1055)

JAVA-4813

* Test expressions in context (#1057)

JAVA-4820

* Add javadoc for boolean, date, number, integer, and expression (#1059)

 JAVA-4799

* Update and add documentation (#1059)

* Fix, tests

 JAVA-4799

* Add `@MqlUnchecked` and a few usage examples (#1059)

 JAVA-4799

* Add has to document, add tests (#1070)

 JAVA-4799

* Add javadocs for remaining classes (#1070)

 JAVA-4799

* 5.2 annotations (#1070)

 JAVA-4799

* 5.0 annotations (#1070)

 JAVA-4799

* 4.4 annotations (#1070)

 JAVA-4799

* 4.2 annotations (#1070)

 JAVA-4799

* 4.0 annotations (#1070)

 JAVA-4799

* Update and add documentation, add tests, fix minor issues (#1070)

Rename extractBsonValue

Fix access modifiers

Remove excess comments

Update docs

Fix: behaviour of get

Add notNull to API, add notNullApi test

Fix docs/annotations, tests

Fix docs, annotations, since

Fix docs

Revert external

Add missing MqlUnchecked

Fix missing null checks

Checkstyle

JAVA-4799

* Rename to Mql (automated) (#1073)

JAVA-3879

* Rename methods (automated) (#1073)

JAVA-3879

* Update naming, terms, and missing checks and annotations (#1073)

JAVA-3879

---------

Co-authored-by: Valentin Kovalenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyemin jyemin jyemin approved these changes

@stIncMale stIncMale stIncMale approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@katcharov @jyemin @stIncMale