- Notifications
You must be signed in to change notification settings - Fork 4
feat: FIR-43724 implement streaming in node sdk #137
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: FIR-43724 implement streaming in node sdk #137
Conversation
abe20e9 to a27943d Compare
ptiurin left a comment
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.
Also, let's modify the README to have the new streaming example.
stepansergeevitch left a comment
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.
Let's add more integration tests
- Let's query all data types and make sure they are all parsed and converted correctly. See https://github.com/firebolt-db/firebolt-go-sdk/blob/main/fixtures/all_types_query.sql for V2 example and https://github.com/firebolt-db/firebolt-go-sdk/blob/main/fixtures/all_types_query_v0.sql for V1 example of a query
- Have a syntax error and check that streaming handles it correctly.
| Please also make sure to update the README.md documentation if needed for streaming |
I have added syntax error and streaming error integration tests to this PR, while I created another task to add type fetching tests since there are some issues |
stepansergeevitch left a comment
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.
One thing I see missing is support for numeric as an alias for Decimal. We need to add it so that numeric values are converted into BigNumber in the same way Decimal values are. We also need to extend support for bigint and double precision type aliases.
goprean left a comment
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.
lgtm (although I am novice in node :-) )
stepansergeevitch left a comment
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.
LGTM. Please fix the comment by @goprean
ptiurin left a comment
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.
Please have a look at Sonar issues.
Apart from that lgtm.
|



No description provided.