Skip to content

Conversation

@LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Nov 3, 2022

What changes were proposed in this pull request?

This pr aims to replace TypeCheckFailure by DataTypeMismatch in type checks in the aggregate expressions:

  • Count
  • CollectSet
  • CountMinSketchAgg
  • HistogramNumeric

Why are the changes needed?

Migration onto error classes unifies Spark SQL error messages.

Does this PR introduce any user-facing change?

Yes. The PR changes user-facing error messages.

How was this patch tested?

Pass GitHub Actions

@LuciferYang LuciferYang changed the title [SPARK-40769][SQL] Migrate type check failures of aggregate expressions onto error classes [WIP][SPARK-40769][SQL] Migrate type check failures of aggregate expressions onto error classes Nov 3, 2022
@LuciferYang LuciferYang changed the title [WIP][SPARK-40769][SQL] Migrate type check failures of aggregate expressions onto error classes [WIP][SPARK-40769][CORE][SQL] Migrate type check failures of aggregate expressions onto error classes Nov 3, 2022
@LuciferYang
Copy link
Contributor Author

LuciferYang commented Nov 3, 2022

Still in wip, will fix test later

@LuciferYang LuciferYang changed the title [WIP][SPARK-40769][CORE][SQL] Migrate type check failures of aggregate expressions onto error classes [SPARK-40769][CORE][SQL] Migrate type check failures of aggregate expressions onto error classes Nov 3, 2022
"expectedNum" -> " >= 1",
"actualNum" -> "0",
"legacyNum" -> "0",
"legacyConfKey" -> SQLConf.ALLOW_PARAMETERLESS_COUNT.key,
Copy link
Member

Choose a reason for hiding this comment

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

Please, use toSQLConf() to quote the config.

"actualNum" -> "0",
"legacyNum" -> "0",
"legacyConfKey" -> SQLConf.ALLOW_PARAMETERLESS_COUNT.key,
"legacyConfValue" -> true.toString
Copy link
Member

Choose a reason for hiding this comment

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

true should be quoted by "", see error-classes.json. For instance,

"<message>.<alternative> If necessary set <config> to \"false\" to bypass this error."

I would add new helper toSQLConfVal() like:

  def toSQLConfVal(conf: String): String = {
    quoteByDefault(conf)
  }

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

Waiting for CI.

@LuciferYang
Copy link
Contributor Author

GA passed

@MaxGekk
Copy link
Member

MaxGekk commented Nov 4, 2022

+1, LGTM. Merging to master.
Thank you, @LuciferYang.

@MaxGekk MaxGekk closed this in a2a8de9 Nov 4, 2022
SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
…ressions onto error classes

### What changes were proposed in this pull request?
This pr aims to replace `TypeCheckFailure` by `DataTypeMismatch` in type checks in the aggregate expressions:

- Count
- CollectSet
- CountMinSketchAgg
- HistogramNumeric

### Why are the changes needed?
Migration onto error classes unifies Spark SQL error messages.

### Does this PR introduce _any_ user-facing change?
Yes. The PR changes user-facing error messages.

### How was this patch tested?
Pass GitHub Actions

Closes apache#38498 from LuciferYang/SPARK-40769.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
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.

2 participants