-
Notifications
You must be signed in to change notification settings - Fork 236
feat(export-to-language): Add Go Support #2991
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.
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
feat(export-to-language): Add Go Support #2991
Conversation
…f github.com:prestonvasquez/compass into COMPASS-5685.addVariableDeclarationsToBSONTranspiler
This reverts commit 1f58923.
Co-authored-by: Anna Henningsen <github@addaleax.net>
Co-authored-by: Anna Henningsen <github@addaleax.net>
…f github.com:prestonvasquez/compass into COMPASS-5685.addVariableDeclarationsToBSONTranspiler
…f github.com:prestonvasquez/compass into COMPASS-5685.addVariableDeclarationsToBSONTranspiler
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
…quez/compass into GODRIVER-2268.compassGoExport
if (literal === '') | ||
return 'bson.A{}'; | ||
return `bson.A{${literal}${closingIndent}}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the generated code for aggregation pipelines uses a top-level bson.A
instead of a mongo.Pipeline
. Most of the Go driver documentation shows using a mongo.Pipeline
for Aggregate()
if all of the pipeline stages are bson.D
values (see an example here).
Is it possible to use a mongo.Pipeline
instead of a bson.A
for the top-level pipeline value in Aggregate()
calls?
E.g.
mongo.Pipeline{
bson.D{{"$sort", bson.D{{"x", 1}}}},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost certainly possible, but at the moment there is no wrapper specifically for aggregations, it just treats []
as whatever our transpiler defines for the ArrayTypeTemplate
. Looking at the go-driver aggregation code it seems that bson.A
will work in a pinch, but I've created COMPASS-5742 to investigate adding a method to propagate aggregation/filter metadata to the compile methods to create a switch condition to help with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to defer to COMPASS-5742.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Go code looks good to me.
In some examples you ignore the result of a Find
or Aggregate
operation (I'm assuming because it introduces an unused variable), but it should be pretty obvious to users that they should replace _
with their own variable and continue logic with that variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a few bugs, but everything else looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome! 👍
############################################# | ||
|
||
DriverTemplate: &DriverTemplate !!js/function > | ||
(spec) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to wrap all of this in a main()
function so that it can be copied and pasted to run without needing to add overhead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other languages don't seem to do that, cc: @mmarcon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcasimir and I chatted about this. The other languages don't do this but they probably should. A good UX compromise could be to wrap the code inside a function RunAggregation
or something like that which then a user can call from their main()
. Maybe let's do that as a follow-up PR.
…quez/compass into GODRIVER-2268.compassGoExport
Description
GODRIVER-2268
This adds the Go language as an output for the language transpiler, enabling exports for find and aggregation pipelines.
For the dbx-go team, to test on compass locally you can read the instructions for installing here:
brew install node@^14.17.5
npm install -g npm7
git checkout prestonvasquez:GODRIVER-2268.compassGoExport
npm run bootstrap
npm run start
Checklist
Motivation and Context
Open Questions
There are a few open questions from the dbx-go team:
Q1. Is it possible to do the string parsing within the transpiler code?. This is in reference to how data is transpiled when trying to convert a string to a long type. In its current state the code will generate a function to handle this:
But perhaps it would be better to just cast the literal directly:
Two considerations for casting directly:
Q2. Are there any cases where the query might contain NumberLong(variable)? This is a follow-up to Q1.
Q3. Also, is it possible to just print an error message, like "unsupported conversion"? This is also a follow-up to Q1. If we are not wanting to include the function in the raw output, should we just not support any raw output that require declarations?
Q4. If we decide to use declarations, do we want them to be functions or variables?
Dependents
This ticket depends on #2964 , which adds the capability of prepending statements with declarations.
Types of changes