Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Dec 8, 2019

What changes were proposed in this pull request?

  • Replace Seq[String] by Seq[_] in StopWordsRemoverSuite because String type is unchecked due erasure.
  • Throw an exception for default case in MLTest.checkNominalOnDF because we don't expect other attribute types currently.
  • Explicitly cast float to double in BigDecimal(y). This is what the apply() method does for floats.
  • Replace deprecated verifyZeroInteractions by verifyNoInteractions.
  • Equivalent replacement of \0 by \u0000 in CSVExprUtilsSuite
  • Import scala.language.implicitConversions in CollectionExpressionsSuite, HashExpressionsSuite and in ExpressionParserSuite.

Why are the changes needed?

The changes fix compiler warnings showed in the JIRA ticket https://issues.apache.org/jira/browse/SPARK-30170 . Eliminating the warning highlights other warnings which could take more attention to real problems.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites StopWordsRemoverSuite, AnalysisExternalCatalogSuite, CSVExprUtilsSuite, CollectionExpressionsSuite, HashExpressionsSuite, ExpressionParserSuite and sub-tests of MLTest.

@SparkQA
Copy link

SparkQA commented Dec 8, 2019

Test build #114995 has finished for PR 26799 at commit ce83a11.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

case binAttr: BinaryAttribute => Some(2)
case nomAttr: NominalAttribute => nomAttr.getNumValues
case unknown =>
throw new IllegalArgumentException(s"Attribute type: ${unknown.getClass.getName}")
Copy link
Member Author

Choose a reason for hiding this comment

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

    Warning:Warning:line (88)match may not be exhaustive.
It would fail on the following inputs: NumericAttribute(), UnresolvedAttribute
    val n = Attribute.fromStructField(dataframe.schema(colName)) match {

val plan = Project(Seq(func), testRelation)
analyzer.execute(plan)
verifyZeroInteractions(catalog)
verifyNoInteractions(catalog)
Copy link
Member Author

Choose a reason for hiding this comment

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

Warning:Warning:line (62)method verifyZeroInteractions in class Mockito is deprecated: see corresponding Javadoc for more information.
      verifyZeroInteractions(catalog)

import java.util.TimeZone

import scala.util.Random
import scala.language.implicitConversions
Copy link
Member Author

Choose a reason for hiding this comment

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

Warning:Warning:line (39)implicit conversion method stringToUTF8Str should be enabled
by making the implicit value scala.language.implicitConversions visible.
This can be achieved by adding the import clause 'import scala.language.implicitConversions'
or by setting the compiler option -language:implicitConversions.
See the Scaladoc for value scala.language.implicitConversions for a discussion
why the feature should be explicitly enabled.
  implicit def stringToUTF8Str(str: String): UTF8String = UTF8String.fromString(str)

@SparkQA
Copy link

SparkQA commented Dec 9, 2019

Test build #114996 has finished for PR 26799 at commit 9a954fd.

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

@MaxGekk MaxGekk changed the title [WIP][SPARK-30170][SQL] Eliminate compilation warnings: part 1 [SPARK-30170][SQL] Eliminate compilation warnings: part 1 Dec 9, 2019
@dongjoon-hyun
Copy link
Member

Since this PR has both MLLIB and SQL fixes together, I'm wondering what was the criteria for the division part1..partX. Usually, we do this per module or per warnings-type.

@MaxGekk MaxGekk changed the title [SPARK-30170][SQL] Eliminate compilation warnings: part 1 [SPARK-30170][SQL][MLLIB] Eliminate compilation warnings: part 1 Dec 9, 2019
@MaxGekk
Copy link
Member Author

MaxGekk commented Dec 9, 2019

I'm wondering what was the criteria for the division part1..partX

I was trying to fix everything except of warnings in Kafka connector, Parquet datasource and deprecation warnings from Spark itself. Other parts should cover those warnings.

@srowen
Copy link
Member

srowen commented Dec 9, 2019

Up to you about what is most efficient. If it's a bunch of misc tiny issues, you can put them together. The only thing I try to do is not split solutions to the same class of problem over two PRs if it's easy not to. For example for Scala 2.13 there are like 10+ distinct types of change. For anything important or touching many files I do one PR. But for the last few that involve a file or two each will probably batch them together.

@MaxGekk
Copy link
Member Author

MaxGekk commented Dec 9, 2019

For anything important or touching many files I do one PR. But for the last few that involve a file or two each will probably batch them together.

I did exactly the same. Originally I tried to fix warnings in JSON datasource here but I saw they became heavy, and put them to the separate PR: #26797 . Other fixes are comparably small, and put all of them to this PR.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Hi, @MaxGekk .
Let's revert sql/catalyst/src/main/scala/org/apache/spark/sql/types/FloatType.scala and put [TESTS] into the PR title. For the others, looks good.

@MaxGekk
Copy link
Member Author

MaxGekk commented Dec 11, 2019

Let's revert sql/catalyst/src/main/scala/org/apache/spark/sql/types/FloatType.scala

Just in case, @srowen are you ok to revert this?

@dongjoon-hyun
Copy link
Member

@MaxGekk . It's irrelevant to the @srowen 's patch which is merged already.
I mean reverting your change in this PR and leave the warning AS-IS for non-test code.

@srowen srowen changed the title [SPARK-30170][SQL][MLLIB] Eliminate compilation warnings: part 1 [SPARK-30170][SQL][MLLIB][TESTS] Eliminate compilation warnings: part 1 Dec 12, 2019
@srowen srowen closed this in 25de90e Dec 12, 2019
@srowen
Copy link
Member

srowen commented Dec 12, 2019

Merged to master. I have some questions about the umbrella JIRA: I think these need more descriptive names and I'm not sure all of those warnings can be resolved without removing tests for code we still need to test.

@MaxGekk MaxGekk deleted the eliminate-warning-2 branch June 5, 2020 19:41
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.

4 participants