-
Notifications
You must be signed in to change notification settings - Fork 258
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 column types when default column is being patched for missing data features #1188
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nit comments / request for additional unit tests.
...rc/main/scala/com/linkedin/feathr/offline/transformation/FeatureValueToColumnConverter.scala
Outdated
Show resolved
Hide resolved
feathr-impl/src/test/scala/com/linkedin/feathr/offline/AnchoredFeaturesIntegTest.scala
Show resolved
Hide resolved
feathr-impl/src/test/scala/com/linkedin/feathr/offline/AnchoredFeaturesIntegTest.scala
Outdated
Show resolved
Hide resolved
feathr-impl/src/test/scala/com/linkedin/feathr/offline/AnchoredFeaturesIntegTest.scala
Outdated
Show resolved
Hide resolved
assertEquals(featureList(0).getAs[Row]("featureWithNull7"), null) | ||
assertEquals(featureList(0).getAs[Row]("featureWithNull"),-1.0f) | ||
assertEquals(featureList(0).getAs[Row]("featureWithNull4"),Map()) | ||
assertEquals(featureList(0).getAs[Row]("featureWithNull4"), null) | ||
assertEquals(featureList(0).getAs[Row]("featureWithNull2"),1.0f) | ||
assertEquals(featureList(0).getAs[Row]("derived_featureWithNull"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should have failed right? As derived_featureWithNull
is computed from featureWithNull
which itself is a NUMERIC type. Do we still expect the type of derived features from missed anchored features as
string -> float?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, the @test annotation was removed from this test, so the test was not being run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I still had this question, why is derived_featureWithNull
a tensor when we know the type of featureWithNull
is NUMERIC? Is the right expectation?
feathr-impl/src/test/scala/com/linkedin/feathr/offline/SlidingWindowAggIntegTest.scala
Show resolved
Hide resolved
feathr-impl/src/main/scala/com/linkedin/feathr/offline/swa/SlidingWindowAggregationJoiner.scala
Show resolved
Hide resolved
feathr-impl/src/main/scala/com/linkedin/feathr/offline/swa/SlidingWindowAggregationJoiner.scala
Show resolved
Hide resolved
...-impl/src/main/scala/com/linkedin/feathr/offline/join/workflow/AnchoredFeatureJoinStep.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fix column types when default column is being patched for missing data features in both anchored and SWA features.