Skip to content
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

Add query building API for the $fill aggregation pipeline stage #965

Merged
merged 5 commits into from Jul 6, 2022
Merged

Add query building API for the $fill aggregation pipeline stage #965

merged 5 commits into from Jul 6, 2022

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale commented Jun 16, 2022 •
edited

The new API:

  • WindowedComputations.locf for $locf
  • WindowedComputations.linearFill for $linearFill
  • Aggregates.fill for $fill

The server-side syntax document is here.

You may see how the API usage looks like, or play with it in

  • AggregatesFunctionalSpecification.'$setWindowFields'
  • AggregatesFunctionalSpecification.'$fill'

JAVA-4394

@stIncMale stIncMale requested a review from jyemin June 16, 2022 19:17
@stIncMale stIncMale self-assigned this Jun 16, 2022
@stIncMale stIncMale requested a review from katcharov June 16, 2022 19:54
@stIncMale stIncMale marked this pull request as draft June 21, 2022 02:05
@stIncMale stIncMale marked this pull request as ready for review June 23, 2022 21:45
@stIncMale stIncMale marked this pull request as draft June 25, 2022 00:40
@stIncMale stIncMale requested review from rozza and removed request for jyemin June 28, 2022 01:49
@stIncMale stIncMale marked this pull request as ready for review June 28, 2022 01:50
@stIncMale
Copy link
Member Author

stIncMale commented Jun 28, 2022 •
edited

The PR was refactored. A single pipeline stage specification is no longer used. Instead

  • the API uses an object that represents optional fields (similarly to Aggregates.bucket, Aggregates.search; the API of this object has the same style as the API of SearchOptions);
  • the API uses varargs to represents output, similarly to Aggregates.setWindowFields.

rozza
rozza reviewed Jun 29, 2022
View reviewed changes
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

This is a tricky API as the syntax seems to suggest optional values first - which is different to our (Java driver) general approach.

I found the name FillComputation surprising as it represents the output field in the syntax. Did you try any other names? I can only suggest FillOutput then type name is more closely linked to the field it represents.

@stIncMale
Copy link
Member Author

This is a tricky API as the syntax seems to suggest optional values first - which is different to our (Java driver) general approach.

This is only true if we want to accept output as varargs. This is not a new situation for us, and we had exactly the same situation in Aggregates.setWindowFields which, in turn, was modelled after Aggregates.group, that accepts its output via varargs (this part is not called output in the MQL, which only tells us that the MQL is also inconsistent with itself). One may also observe that Aggregates.bucket accepts its output via BucketOptions.output, which is different from Aggregates.group.

I found the name FillComputation surprising as it represents the output field in the syntax. Did you try any other names? I can only suggest FillOutput then type name is more closely linked to the field it represents.

We had this exact discussion for Aggregates.setWindowFields, because WindowedComputation also seemed weird, yet WindowedOutput/WindowOutput appears to be even weirder. I constructed the name FillComputation to be similar to WindowedComputation. If we rename FillComputation to FillOutput, then we should also rename WindowedComputation to WindowedOutput (or WindowOutput 🤷‍♂️). The upcoming release is the last point where we can do this, because Aggregates.setWindowFields goes out of beta in 4.7.0.

Ross, is your proposal to rename WindowedComputation/FillComputation to WindowedOutput/FillOutput?

@rozza
Copy link
Member

rozza commented Jun 29, 2022

I agree the naming is difficult / problematic - sorry to rehash old ground. I noticed setWindowFields uses the param name output for WindowedComputation.

Looking at the naming:

  • WindowedComputation - Relates to the context but uses new language.
  • WindowFieldOutput - Relates to MQL but awkward to read.
  • WindowOutputField - Relates to MQL
  • WindowOutput - Relates to MQL but is very vague (meaningless?).

Personally I think I prefer: FillOutputField to FillComputation and WindowOutputFieldto WindowedComputation.

Regardless of naming I'll rereview in the morning. I'm also happy to delay naming decisions (as a separate ticket) until Jeff is back to see if we can get consensus.

@stIncMale
Copy link
Member Author

I am fine with FillOutputField/WindowOutputField.

I'm also happy to delay naming decisions (as a separate ticket) until Jeff is back to see if we can get consensus.

Let's defer: JAVA-4665.

@stIncMale stIncMale requested a review from rozza June 29, 2022 18:53
rozza
rozza approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM!

I had one question that is just for my learning - no action needed.

...r-core/src/test/functional/com/mongodb/client/model/AggregatesFunctionalSpecification.groovy Outdated
.append('doc', new Document('field2', 3))]
getCollectionHelper().insertDocuments(docs)
String resultField = 'result'
LinkedList<Bson> stages = [
Copy link
Member

Choose a reason for hiding this comment

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

Q - why linkedlist over just List?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I could have get by with the List interface. Done.

katcharov
katcharov approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

lgtm

Just a trivial doc suggestion.

driver-core/src/main/com/mongodb/client/model/Aggregates.java Outdated
}

/**
* Creates a {@code $fill} pipeline stage, which sets values to fields when they are {@link BsonType#NULL Null} or missing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Creates a {@code $fill} pipeline stage, which sets values to fields when they are {@link BsonType#NULL Null} or missing.
* Creates a {@code $fill} pipeline stage, which assigns values to fields when they are {@link BsonType#NULL Null} or missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. This change was also needed in the Scala code.

@stIncMale
Improve wording
040a94c 
JAVA-4394
@stIncMale stIncMale merged commit c70c2d2 into mongodb:master Jul 6, 2022
@stIncMale stIncMale deleted the JAVA-4394 branch July 6, 2022 23:21
@stIncMale stIncMale restored the JAVA-4394 branch July 6, 2022 23:22
@stIncMale stIncMale deleted the JAVA-4394 branch July 17, 2022 00:03
ispringer pushed a commit to evergage/mongo-java-driver that referenced this pull request May 16, 2023
@stIncMale @ispringer
Add query building API for the $fill aggregation pipeline stage (mo…
ad8799c 
…ngodb#965)

JAVA-4394
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rozza rozza rozza approved these changes

@katcharov katcharov katcharov approved these changes

Assignees

@stIncMale stIncMale

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@stIncMale @rozza @katcharov