Skip to content

Conversation

@jsoref
Copy link
Contributor

@jsoref jsoref commented Dec 9, 2020

What changes were proposed in this pull request?

Consistently correct the spelling of PushedFilters

Why are the changes needed?

@bersprockets noted that it's wrong

Does this PR introduce any user-facing change?

Technically, I think it does. Practically, neither Google nor GitHub show anyone using pushedFilers outside of forks (or the discussion about fixing it started at #30323 (comment))

How was this patch tested?

None beyond CI in the previous PR

@HyukjinKwon HyukjinKwon changed the title spelling: [API?] filters - PushedFilers [MINOR][SQL] Spelling: filters - PushedFilers Dec 9, 2020
@jsoref jsoref mentioned this pull request Dec 11, 2020
@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Mar 19, 2021
@jsoref
Copy link
Contributor Author

jsoref commented Mar 19, 2021

Thanks bot

Signed-off-by: Josh Soref <[email protected]>
@jsoref jsoref force-pushed the spelling-filters branch from 903d13e to 3137298 Compare March 19, 2021 03:01
@jsoref
Copy link
Contributor Author

jsoref commented Mar 19, 2021

@srowen / @bersprockets: anything I need to do to push this along?

@srowen
Copy link
Member

srowen commented Mar 19, 2021

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Mar 19, 2021

Test build #136265 has started for PR 30678 at commit 3137298.

@SparkQA
Copy link

SparkQA commented Mar 19, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40846/

@SparkQA
Copy link

SparkQA commented Mar 19, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40846/

@github-actions github-actions bot closed this Mar 20, 2021
@srowen srowen reopened this Mar 20, 2021
@SparkQA
Copy link

SparkQA commented Mar 20, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40859/

@SparkQA
Copy link

SparkQA commented Mar 20, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40859/

@SparkQA
Copy link

SparkQA commented Mar 20, 2021

Test build #136277 has finished for PR 30678 at commit 3137298.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Mar 20, 2021

Sorry I'm getting to this late - and this is probably an ignorant question, but would this cause any user code to break? like would existing code be setting this under the wrongly-named key at the moment? Just want to make sure we don't break things.

@github-actions github-actions bot closed this Mar 21, 2021
@jsoref
Copy link
Contributor Author

jsoref commented Mar 21, 2021

@srowen: I don't know - nor am I particularly certain about how to check, but apparently the bot is very strongly opinionated.

@srowen
Copy link
Member

srowen commented Mar 21, 2021

Ignore the bot, it's just doing housekeeping. That's my only concern - would this break any user code? is this property even observable or settable by users?

@jsoref
Copy link
Contributor Author

jsoref commented Mar 21, 2021

I can't speak definitively, but the most prominent hits are this PR itself or other references to apache/spark internals.

@MaxGekk added this in #29145 in July of 2020, @gengliangwang, @HeartSaVioR, @bersprockets, and @HyukjinKwon seem to be the people most likely to be able to give an educated opinion (and notably, @bersprockets is the one who suggested doing this).

@MaxGekk
Copy link
Member

MaxGekk commented Mar 21, 2021

The method getMetaData() is invoke from FileScan.description() and DataSourceV2ScanExecBase.verboseStringWithOperatorId() that print "internal" info about plan nodes. I think that the risk of breaking users apps is low.

@MaxGekk MaxGekk reopened this Mar 21, 2021
@SparkQA
Copy link

SparkQA commented Mar 21, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40886/

@SparkQA
Copy link

SparkQA commented Mar 21, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40886/

@SparkQA
Copy link

SparkQA commented Mar 21, 2021

Test build #136304 has finished for PR 30678 at commit 3137298.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MaxGekk
Copy link
Member

MaxGekk commented Mar 21, 2021

@jsoref Could you update PR's description, for instance fill in What changes were proposed in this pull request?.

@github-actions github-actions bot closed this Mar 22, 2021
@jsoref
Copy link
Contributor Author

jsoref commented Mar 22, 2021

@MaxGekk: Done (and the bot seems to hate me)

@HeartSaVioR HeartSaVioR removed the Stale label Mar 22, 2021
@HeartSaVioR HeartSaVioR reopened this Mar 22, 2021
@HeartSaVioR
Copy link
Contributor

(Note to committers; when reopening, please remove Stale tag as well, which is a marker Github Bot leverages to check and close the PR.)

@SparkQA
Copy link

SparkQA commented Mar 22, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40900/

@SparkQA
Copy link

SparkQA commented Mar 22, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40900/

Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

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

LGTM

@MaxGekk
Copy link
Member

MaxGekk commented Mar 22, 2021

+1, LGTM, I am merging this to master.

@MaxGekk MaxGekk closed this in f4de93e Mar 22, 2021
@SparkQA
Copy link

SparkQA commented Mar 22, 2021

Test build #136318 has finished for PR 30678 at commit 3137298.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jsoref jsoref deleted the spelling-filters branch March 22, 2021 12:31
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.

6 participants