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.

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

SNOW-1332640 support reading of structured types #870

Closed
wants to merge 25 commits into from

Conversation

sfc-gh-pmotacki
Copy link
Collaborator

Description

Please explain the changes you made here.

Checklist

  • Format code according to the existing code style (run npm run lint:check -- CHANGED_FILES and fix problems in changed code)
  • Create tests which fail without the change (if possible)
  • Make all tests (unit and integration) pass (npm run test:unit and npm run test:integration)
  • Extend the README / documentation and ensure is properly displayed (if necessary)
  • Provide JIRA issue id (if possible) or GitHub issue id in commit message

@sfc-gh-pmotacki sfc-gh-pmotacki requested a review from a team as a code owner July 18, 2024 08:31
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
test/integration/testStructuredType.js Outdated Show resolved Hide resolved
test/integration/testStructuredType.js Outdated Show resolved Hide resolved
test/integration/testStructuredType.js Outdated Show resolved Hide resolved
@sfc-gh-pmotacki sfc-gh-pmotacki changed the title Snow 1332640 structured types support SNOW-1332640 support reading of structured types Jul 23, 2024
@sfc-gh-pmotacki sfc-gh-pmotacki force-pushed the SNOW-1332640-structured-types-support branch 4 times, most recently from e69b365 to 9d3987a Compare August 2, 2024 07:13
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 91.45299% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.99%. Comparing base (a33f997) to head (0fbb24e).

Files with missing lines Patch % Lines
lib/connection/result/column.js 92.64% 5 Missing ⚠️
lib/connection/result/datetime_format_converter.js 91.66% 3 Missing ⚠️
lib/connection/result/data_types.js 80.00% 1 Missing ⚠️
lib/connection/result/sf_timestamp.js 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #870      +/-   ##
==========================================
+ Coverage   88.88%   88.99%   +0.11%     
==========================================
  Files          65       66       +1     
  Lines        6304     6409     +105     
==========================================
+ Hits         5603     5704     +101     
- Misses        701      705       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/column.js Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/column.js Outdated Show resolved Hide resolved
lib/connection/result/sf_timestamp.js Outdated Show resolved Hide resolved
value = convertTimestampTzString(columnValue, formatLtz, context.statementParameters['TIMEZONE'], column.getScale()).toSfDate();
break;
case 'timestamp_ntz':
// context.format = context.statementParameters['TIMESTAMP_NTZ_OUTPUT_FORMAT'] || context.statementParameters['TIMESTAMP_OUTPUT_FORMAT'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we delete this comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the file naming convention? I see that we use snake_case and camelCase at the same time

);
});

// it('test nested object', function (done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why commented? do we need this?

@@ -46,7 +46,7 @@ describe('Connection test', function () {
assert.ok(!connection.isUp(), 'still active');
});

it('Wrong Username', function (done) {
it.skip('Wrong Username', function (done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we skip this test?

complete: function (err, stmt, rows) {
testUtil.checkError(err);
const row = rows[0];
const narmalizedRow = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo narmalized instead of normalized.

complete: function (err, stmt, rows) {
testUtil.checkError(err);
const row = rows[0];
const narmalizedRow = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo

@sfc-gh-pmotacki sfc-gh-pmotacki force-pushed the SNOW-1332640-structured-types-support branch from 33507d1 to 0fbb24e Compare September 11, 2024 17:57
@sfc-gh-pmotacki
Copy link
Collaborator Author

Added to changes in feather branch - PR #874

@github-actions github-actions bot locked and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants