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

[CDAP-15361] Add feature flag for schema management #654

Merged
merged 1 commit into from
Aug 23, 2023
Merged

[CDAP-15361] Add feature flag for schema management #654

merged 1 commit into from
Aug 23, 2023

Conversation

vanathi-g
Copy link
Contributor

@vanathi-g vanathi-g commented Jul 26, 2023 •
edited
Loading

Changes

  • PR in CDAP: [CDAP-15361] Feature flag for schema management in Wrangler cdapio/cdap#15256
  • ExecutorContext now has a new field to check whether schema handling is enabled (set as true in ServicePipelineContext and TestingPipelineContext)
  • The feature flag is also checked if enabled in
    • AbstractDirectiveHandler - UI display of types (execution response)
    • WorkspaceHandler - generating plugin output schema in specification
  • If the feature flag is disabled, fallback to old schema inference logic
  • Also fixes CDAP-20740

Testing

  • Moved createUberRecord from AbstractDirectiveHandler to a new RowHelper utility class to ensure the same method is used consistently (including in tests)
  • Refactored existing schema tests
  • Added an additional test to check output schema inference when schema management is disabled

@vanathi-g vanathi-g added the build Triggers unit test build label Jul 26, 2023
tivv
tivv reviewed Aug 15, 2023
View reviewed changes
Copy link
Contributor

@tivv tivv left a comment

Choose a reason for hiding this comment

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

Please also add/update tests

wrangler-core/pom.xml Outdated Show resolved Hide resolved
@vanathi-g vanathi-g requested a review from tivv August 21, 2023 08:30
tivv
tivv reviewed Aug 22, 2023
View reviewed changes
wrangler-core/src/main/java/io/cdap/wrangler/utils/RowHelper.java Outdated Show resolved Hide resolved
wrangler-core/src/test/java/io/cdap/wrangler/TestingPipelineContext.java Outdated
return schemaManagementEnabled;
}

protected class MockStageMetrics implements StageMetrics {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Any reason not to use Mockito? Much less boilerplate code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. I'd just moved the original code from the constructor to it's own class but I've used mockito now

wrangler-core/src/test/java/io/cdap/wrangler/TestingPipelineContext.java Outdated Show resolved Hide resolved
wrangler-core/src/test/java/io/cdap/wrangler/TestingPipelineContext.java Outdated Show resolved Hide resolved
@vanathi-g vanathi-g requested a review from tivv August 23, 2023 13:51
@vanathi-g vanathi-g merged commit 3ef9739 into develop Aug 23, 2023
3 checks passed
@vanathi-g vanathi-g deleted the schema-ff branch August 23, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tivv tivv tivv approved these changes

@saimukkamala saimukkamala Awaiting requested review from saimukkamala

Assignees
No one assigned
Labels
build Triggers unit test build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vanathi-g @tivv