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

getSnowflakeType() could be more efficient #881

Closed
Yifeng-Sigma opened this issue Aug 8, 2023 · 4 comments · Fixed by #890
Closed

getSnowflakeType() could be more efficient #881

Yifeng-Sigma opened this issue Aug 8, 2023 · 4 comments · Fixed by #890
Assignees
@sfc-gh-anugupta
Labels
enhancement The issue is a request for improvement or a new feature

Comments

@Yifeng-Sigma
Copy link
Contributor

Yifeng-Sigma commented Aug 8, 2023 •
edited
Loading

gosnowflake/datatype.go

Lines 47 to 53 in ad2d6fe

func getSnowflakeType(typ string) snowflakeType {
for i, sft := range snowflakeTypes {
if sft == typ {
return snowflakeType(i)
} else if snowflakeType(i) == nullType {
break
}

why not use a map here to have O(1) lookup?
@Yifeng-Sigma Yifeng-Sigma added the bug Erroneous or unexpected behaviour label Aug 8, 2023
@sfc-gh-dszmolka sfc-gh-dszmolka self-assigned this Aug 9, 2023
@sfc-gh-dszmolka sfc-gh-dszmolka added status-triage Issue is under initial triage status-information_needed Additional information is required from the reporter and removed bug Erroneous or unexpected behaviour labels Aug 9, 2023
@sfc-gh-dszmolka
Copy link
Contributor

sfc-gh-dszmolka commented Aug 9, 2023 •
edited
Loading

(edited the original comment after syncing a bit internally with the rest of the team) it looks like this is a good improvement, thank you for raising this Issue with us. probably won't bring a huge performance impact, but definitely something which makes sense to implement. We'll take a look and update this one with the progress.

@sfc-gh-dszmolka sfc-gh-dszmolka added enhancement The issue is a request for improvement or a new feature status-in_progress Issue is worked on by the driver team and removed status-triage Issue is under initial triage status-information_needed Additional information is required from the reporter labels Aug 9, 2023
@sfc-gh-dszmolka
Copy link
Contributor

#890

@sfc-gh-dszmolka sfc-gh-dszmolka added status-pr_pending_merge A PR is made and is under review and removed status-in_progress Issue is worked on by the driver team labels Aug 21, 2023
@sfc-gh-dheyman sfc-gh-dheyman mentioned this issue Aug 21, 2023
Merged
6 tasks
@sfc-gh-dszmolka
Copy link
Contributor

sfc-gh-dszmolka commented Aug 24, 2023 •
edited
Loading

apparently the PR is merged :) reopening this issue to be able to track that it will be part of the next release, coming towards end of September (as we just released August's release 1.6.24)

@sfc-gh-dszmolka sfc-gh-dszmolka added status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. enhancement The issue is a request for improvement or a new feature and removed enhancement The issue is a request for improvement or a new feature status-pr_pending_merge A PR is made and is under review labels Aug 24, 2023
@sfc-gh-dszmolka
Copy link
Contributor

released with 1.6.25

@sfc-gh-dszmolka sfc-gh-dszmolka removed the status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. label Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sfc-gh-anugupta sfc-gh-anugupta

Labels
enhancement The issue is a request for improvement or a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

SNOW-889572 Refactored snowflake type implementation to map snowflakedb/gosnowflake
4 participants
@sfc-gh-dszmolka @Yifeng-Sigma @sfc-gh-anugupta @sfc-gh-triage-snowpark-ecosystem-dl