Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 19, 2020

What changes were proposed in this pull request?

This PR fixes the RAT exclusion rule which was originated from SPARK-1144 (Apache Spark 1.0)

Why are the changes needed?

This prevents the situation like #30415.

Currently, it missed catalog directory due to .log rule.

$ dev/check-license
Could not find Apache license headers in the following files:
 !????? /Users/dongjoon/APACHE/spark-merge/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/MetadataColumn.java
 !????? /Users/dongjoon/APACHE/spark-merge/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsMetadataColumns.java

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the CI with the new rule.

@dongjoon-hyun
Copy link
Member Author

cc @rdblue , @HyukjinKwon , @viirya

@dongjoon-hyun
Copy link
Member Author

cc @ScrapCodes for SPARK-1144, too

Comment on lines 46 to 48
.*txt
.*json
.*data
Copy link
Member

Choose a reason for hiding this comment

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

Does it also apply to above .txt, .json, .data?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be safe, yes. Those are also looks suspicious.

@sunchao
Copy link
Member

sunchao commented Nov 19, 2020

nice find @dongjoon-hyun !

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

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

@dongjoon-hyun
Copy link
Member Author

Thanks. @viirya , @HyukjinKwon , @sunchao .
I updated more according to @viirya 's comment.

@github-actions github-actions bot added the SQL label Nov 19, 2020
@dongjoon-hyun dongjoon-hyun changed the title [MINOR][INFRA] Fix rat exclusion not to exclude catalog [MINOR][INFRA][TESTS] Fix rat exclusion not to exclude catalog Nov 19, 2020
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#
Copy link
Member Author

Choose a reason for hiding this comment

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

I created a JIRA because I touch this file inevitabily.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this change, we need to wait for the full test result of GitHub Action.

@dongjoon-hyun dongjoon-hyun changed the title [MINOR][INFRA][TESTS] Fix rat exclusion not to exclude catalog [SPARK-33483][INFRA][TESTS] Fix rat exclusion not to exclude catalog Nov 19, 2020
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-33483][INFRA][TESTS] Fix rat exclusion not to exclude catalog [SPARK-33483][INFRA][TESTS] Fix rat exclusion patterns and add a LICENSE Nov 19, 2020
.*\.avsc
.*\.txt
.*\.json
.*\.data
Copy link
Member Author

Choose a reason for hiding this comment

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

This hides the directory data and metadata.

.*\.txt
.*\.json
.*\.data
.*\.log
Copy link
Member Author

Choose a reason for hiding this comment

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

This hides catalog directory.

.*\.res
flights_tiny.txt.1
over1k
over10k
Copy link
Member

Choose a reason for hiding this comment

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

I see. I checked these are .txt

flights_tiny.txt.1
over1k
over10k
exported_table/*
Copy link
Member

Choose a reason for hiding this comment

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

this is .q

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

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

@ScrapCodes
Copy link
Member

@dongjoon-hyun Thanks for fixing this. My bad, this bug have existed for years.

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

Test build #131314 has finished for PR 30418 at commit b7aa6f9.

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

@HyukjinKwon
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

Test build #131316 has finished for PR 30418 at commit 45658f0.

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

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

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

@dongjoon-hyun
Copy link
Member Author

Thank you, @HyukjinKwon , @viirya , @ScrapCodes , @sunchao .
Merged to master.

@SparkQA
Copy link

SparkQA commented Nov 19, 2020

Test build #131323 has finished for PR 30418 at commit 45658f0.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants