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

chore(warehouse): handle schema change #2654

Merged
merged 12 commits into from
Nov 15, 2022

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented Nov 4, 2022

Description

Schema Test

Use table-driven tests to handle schema change. The benefit here is we can be able to run these tests parallelly.

Handle Schema Change Behaviour

This is applied to the mismatch of data types between the warehouse schema and staging files schema.

Existing behavior

  • When we are trying to do the conversion of the data but we are not able to do it, we used to send nil values to the load files.

Current behavior

  • Throw ErrIncompatibleSchemaConv, when we are trying to do the conversion of the data but we are not able to do it.
  • Throw ErrSchemaConvNotSupported, when we don't support the conversion of the data.

Notion Ticket

https://www.notion.so/rudderstacks/Handle-schema-table-driven-tests-751462cd730d4e4aaf92fba22c575c34

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

warehouse/schema_test.go Outdated Show resolved Hide resolved
warehouse/schema.go Outdated Show resolved Hide resolved
@achettyiitr achettyiitr force-pushed the chore.schema-table_driven-test branch from c196dbc to 40ddf07 Compare November 4, 2022 13:16
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Base: 45.47% // Head: 45.62% // Increases project coverage by +0.14% 🎉

Coverage data is based on head (1eced77) compared to base (5d466d3).
Patch coverage: 78.12% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2654      +/-   ##
==========================================
+ Coverage   45.47%   45.62%   +0.14%     
==========================================
  Files         289      289              
  Lines       48001    48007       +6     
==========================================
+ Hits        21830    21904      +74     
+ Misses      24784    24716      -68     
  Partials     1387     1387              
Impacted Files Coverage Δ
warehouse/errors.go 0.00% <ø> (ø)
warehouse/slave.go 1.08% <0.00%> (-0.01%) ⬇️
warehouse/schema.go 50.28% <100.00%> (+2.45%) ⬆️
services/rsources/handler.go 71.66% <0.00%> (-0.56%) ⬇️
processor/processor.go 85.99% <0.00%> (-0.42%) ⬇️
jobsdb/jobsdb.go 73.61% <0.00%> (+0.29%) ⬆️
jobsdb/backup.go 76.65% <0.00%> (+0.34%) ⬆️
...rvices/streammanager/kafka/client/testutil/util.go 70.87% <0.00%> (+3.88%) ⬆️
processor/stash/stash.go 66.66% <0.00%> (+22.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

warehouse/schema.go Outdated Show resolved Hide resolved
@achettyiitr achettyiitr requested a review from lvrach November 4, 2022 14:08
@achettyiitr achettyiitr force-pushed the chore.schema-table_driven-test branch from 1b4dc9e to 37e1149 Compare November 4, 2022 14:10
@achettyiitr achettyiitr force-pushed the chore.schema-table_driven-test branch from 37e1149 to e06c501 Compare November 4, 2022 14:42
@achettyiitr achettyiitr requested a review from a team November 8, 2022 11:20
Comment on lines 27 to 34
const (
StringDataType = "string"
BooleanDataType = "boolean"
IntDataType = "int"
BigIntDataType = "bigint"
FloatDataType = "float"
JSONDataType = "json"
TextDataType = "text"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const (
StringDataType = "string"
BooleanDataType = "boolean"
IntDataType = "int"
BigIntDataType = "bigint"
FloatDataType = "float"
JSONDataType = "json"
TextDataType = "text"
const (
StringSchemaType SchemaType = "string"
BooleanSchemaType = "boolean"
IntSchemaType = "int"
BigIntSchemaType = "bigint"
FloatSchemaType = "float"
JSONSchemaType = "json"
TextSchemaType = "text"

Copy link
Member Author

Choose a reason for hiding this comment

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

Added these. Also, moved this to the models package.

@achettyiitr achettyiitr changed the title chore(warehouse): handle schema table driven tests chore(warehouse): handle schema change Nov 11, 2022

const (
StringDataType SchemaType = "string"
BooleanDataType SchemaType = "boolean"
Copy link
Member Author

Choose a reason for hiding this comment

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

only the first constant in this group has an explicit type

https://stackoverflow.com/questions/55282615/golangci-lint-constant-explicit-type

Copy link
Contributor

@abhimanyubabbar abhimanyubabbar left a comment

Choose a reason for hiding this comment

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

Minor nit.

warehouse/errors.go Outdated Show resolved Hide resolved
@achettyiitr achettyiitr requested a review from lvrach November 14, 2022 23:42
Copy link
Member

@lvrach lvrach left a comment

Choose a reason for hiding this comment

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

thanks for addressing all the comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants