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

Fix set feature for data blending #309

Merged
merged 1 commit into from Jul 15, 2020

Conversation

x8lucas8x
Copy link
Contributor

This fixes set feature for data blending. I've tested the use cases of using a dimension, a metric and a metric that is also being displayed. Please check tests to see how blending sql is being formatted.

@x8lucas8x x8lucas8x requested review from twheys and a team as code owners July 14, 2020 09:07
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 91.757% when pulling 840f7ed on fix-set-feature-for-data-blending into d1bbd48 on master.

@coveralls
Copy link

coveralls commented Jul 14, 2020

Coverage Status

Coverage increased (+0.04%) to 91.757% when pulling 24532ac on fix-set-feature-for-data-blending into d1bbd48 on master.

are later used for knowing which columns to query on the respective primary and secondary datasets, given the
columns selected for the blender dataset.

:param blender: A DataSetBlender instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to rename this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'll do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

primary_dataset_fields = set(apply_set_dimensions(primary_dataset.fields, filters, primary_dataset))
secondary_dataset_fields = set(apply_set_dimensions(secondary_dataset.fields, filters, secondary_dataset))

primary_dimension_per_blender_dimension = dict(_map_field(
Copy link
Contributor

@gl3nn gl3nn Jul 14, 2020

Choose a reason for hiding this comment

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

Not sure if I prefer this name over blender_to_primary_field_map, I find it a bit confusing. Also, does it only contain dimensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not only. Haha, I was renaming it as you commented on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted that.

@gl3nn
Copy link
Contributor

gl3nn commented Jul 14, 2020

Can you also make sure that you tested it for more than 1 linked dataset? If it works out of the box it probably doesn't need an automated test.

@x8lucas8x x8lucas8x force-pushed the fix-set-feature-for-data-blending branch from 840f7ed to 24532ac Compare July 14, 2020 16:03
@x8lucas8x
Copy link
Contributor Author

Can you also make sure that you tested it for more than 1 linked dataset? If it works out of the box it probably doesn't need an automated test.

I've added tests with a tertiary dataset being blended.

@x8lucas8x x8lucas8x merged commit f193580 into master Jul 15, 2020
@x8lucas8x x8lucas8x deleted the fix-set-feature-for-data-blending branch July 15, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants