Skip to content

Conversation

@bogdantruta-firebolt
Copy link
Contributor

No description provided.

@bogdantruta-firebolt bogdantruta-firebolt requested a review from a team as a code owner April 10, 2025 10:41
@bogdantruta-firebolt bogdantruta-firebolt force-pushed the FIR-43724-add-streaming-to-node-sdk branch from abe20e9 to a27943d Compare April 10, 2025 12:30
Copy link
Contributor

@ptiurin ptiurin left a 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.

Copy link
Collaborator

@stepansergeevitch stepansergeevitch left a 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

  1. 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
  2. Have a syntax error and check that streaming handles it correctly.
@stepansergeevitch
Copy link
Collaborator

Please also make sure to update the README.md documentation if needed for streaming

@bogdantruta-firebolt
Copy link
Contributor Author

Let's add more integration tests

  1. 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
  2. Have a syntax error and check that streaming handles it correctly.

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

Copy link
Collaborator

@stepansergeevitch stepansergeevitch left a 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.

Copy link

@goprean goprean left a 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 :-) )

Copy link
Collaborator

@stepansergeevitch stepansergeevitch left a 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

Copy link
Contributor

@ptiurin ptiurin left a 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.

@bogdantruta-firebolt bogdantruta-firebolt merged commit 6efe9f1 into main Apr 24, 2025
7 checks passed
@bogdantruta-firebolt bogdantruta-firebolt deleted the FIR-43724-add-streaming-to-node-sdk branch April 24, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants