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

Add extraction function to selector dimensional filter to allow lookup on filter #617

Merged
merged 4 commits into from
Jan 31, 2018
Merged

Add extraction function to selector dimensional filter to allow lookup on filter #617

merged 4 commits into from
Jan 31, 2018

Conversation

garyluoex
Copy link
Collaborator

No description provided.

@garyluoex garyluoex force-pushed the NewSelector branch 2 times, most recently from 4d86199 to d70f6fb Compare January 30, 2018 18:58
QubitPi
QubitPi suggested changes Jan 30, 2018
View reviewed changes
fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/DimensionalFilter.java Outdated

this.dimension = dimension;
if (dimension instanceof ExtractionFunctionDimension) {
extractionFunction = ((ExtractionFunctionDimension) dimension).getExtractionFunction().get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure .get() returns a extracFn?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or null

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SelectorFilter.java Outdated
@@ -20,7 +21,19 @@
* @param value Value of the filter
*/
public SelectorFilter(Dimension dimension, String value) {
super(dimension, DefaultFilterType.SELECTOR);
this(dimension, value, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

how about calling super that takes 2 args?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right

...-core/src/test/groovy/com/yahoo/bard/webservice/druid/model/filter/SelectorFilterSpec.groovy
@@ -0,0 +1,45 @@
package com.yahoo.bard.webservice.druid.model.filter
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing header

...-core/src/test/groovy/com/yahoo/bard/webservice/druid/model/filter/SelectorFilterSpec.groovy Outdated
/**
* Test selector filter serialization.
*/
class SelectorFilterSpec extends Specification{
Copy link
Contributor

Choose a reason for hiding this comment

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

space before {

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SelectorFilter.java Outdated
* @param dimension Dimension to apply the extraction to
* @param value Value of the filter
* @param extractionFn Extraction function to be applied on dimension
*
Copy link
Contributor

Choose a reason for hiding this comment

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

extra line

QubitPi
QubitPi approved these changes Jan 30, 2018
View reviewed changes
Copy link
Contributor

@QubitPi QubitPi left a comment

Choose a reason for hiding this comment

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

👍

...-core/src/test/groovy/com/yahoo/bard/webservice/druid/model/filter/SelectorFilterSpec.groovy
@@ -1,3 +1,5 @@
// Copyright 2017 Yahoo Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2018

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yahoo Inc. is not valid also anymore I think

michael-mclawhorn
michael-mclawhorn requested changes Jan 30, 2018
View reviewed changes
Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

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

Seems pretty solid. A few small bugs.

We should follow up with @gyehuda to find out how we should be copywriting oath opensource code going forward.

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/ExtractionFilter.java Outdated
import java.util.Objects;

/**
* Filter for matching a dimension using some specific Extraction function.
*
* @deprecated Use other dimensional filters with extractionFn specified instead
Copy link
Contributor

Choose a reason for hiding this comment

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

Use javadoc {@link} to other class.

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SelectorFilter.java Outdated
*/
public SelectorFilter withExtractionFn(ExtractionFunction extractionFn) {
return new SelectorFilter(getDimension(), value, extractionFn);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Other withers need to be updated to preserve the extractionFn value.

michael-mclawhorn
michael-mclawhorn approved these changes Jan 31, 2018
View reviewed changes
Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

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

Needs a quick rereview.

@gyehuda
Copy link

gyehuda commented Jan 31, 2018

Asserting the name Yahoo Inc. is the correct copyright holder for code published before Yahoo's name was formally changed in June 2017. Between then and January 2018 you could publish new files using Yahoo Inc. or more accurately Yahoo Holdings Inc. Either was fine.

Going forward:

  1. no need to change old copyrights on files you are not changing. Think of the copyright statement like a timestamp, not a freshness stamp. Those files will simply say Yahoo Inc. and the year it was published.
  2. if you publish a new file now, put a new copyright on it. These days the proper copyright for new files in this project is "Copyright 2018 Oath Inc."
  3. if you are making changes to an existing file, you can, but don't have to update the copyright header. This is one of those inconclusive things that we deal with. We don't use date ranges either. So if the change is minor, don't bother changing the copyright. If it's significant, you can if you wish.

At the end of the day, this code is open source under a permissive license. Everyone in the world can use this code. We simply try to be accurate because we care about quality.

@michael-mclawhorn michael-mclawhorn merged commit bcde320 into master Jan 31, 2018
@QubitPi QubitPi mentioned this pull request Mar 6, 2018
Closed
@michael-mclawhorn michael-mclawhorn deleted the NewSelector branch December 12, 2018 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QubitPi QubitPi QubitPi approved these changes

@michael-mclawhorn michael-mclawhorn michael-mclawhorn approved these changes

Assignees
No one assigned
Labels
REVIEWABLE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@garyluoex @gyehuda @michael-mclawhorn @QubitPi