[CARBONDATA-3276] Compacting table that do not exist should modify the message of MalformedCarbonCommandException #3106
Conversation
|
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2463/ |
|
Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10720/ |
|
Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2690/ |
integration/spark2/src/main/scala/org/apache/spark/sql/execution/strategy/DDLStrategy.scala
Outdated
Show resolved
Hide resolved
…uchTableException instead of MalformedCarbonCommandException
qiuchenjian
force-pushed
the
NoSuchTable
branch
from
07cc348
to
ad981a0
Compare
|
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2495/ |
|
Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10753/ |
|
Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2724/ |
| @InterfaceAudience.User | ||
| @InterfaceStability.Stable | ||
| public class NoSuchCarbonTableException extends MalformedCarbonCommandException { | ||
| public NoSuchCarbonTableException(String db, String table) { |
There was a problem hiding this comment.
I feel this PR may not be required
- Spark already provides
NoSuchTableException. We can use the same to throw the error if required - You are making the use of this exception in DDLStrategy. In DDLstrategy we specifically check for CarbonTable. If CarbonTable is not found then it can also be a parquet or hive table. So I think either we should continue with current malformed exception being thrown or we should pass the flow to hive/parquet and let the exception be thrown from there.
There was a problem hiding this comment.
- i think MalformedCarbonCommandException is not right, user will not know the real exception
- the ddl is used for carbon, so i don't think we should pass the flow to hive
- Do you think which is better? NoSuchTableException or NoSuchCarbonTableException , or some better suggests?
There was a problem hiding this comment.
@qiuchenjian ..
- Carbon in one of the component integrated with spark just like hive and we can multiple components integrated together in one system with spark. In
DDLStratgeywe are just tapping the commands to check for the carbon table. If its a carbon table we give to carbon flow else we give it to spark. We are throwing Malformed exception because spark itself does not support the Alter operation. - Still If you want to change the Malformed exception you can change it to
NoSuchTableExcpetion. Its a generic exception and should be sufficient.
There was a problem hiding this comment.
@manishgupta88 I still suggest to mention user more clearly, i change it to NoSuchTableException, please review again, thank you
There was a problem hiding this comment.
@manishgupta88 discuss with david (@QiangCai ), he suggest to use MalformedCarbonCommandException and modify the message to "Table or view 'no_table' not found in database 'default' or not carbon format"
There was a problem hiding this comment.
@qiuchenjian ...This was the very first comment that we throw MalformedCarbonCommandException for a reason and the PR may not be required but you were not convinced. Now that you have taken another opinion you can make the required changes and get it reviewed and merged
…uchTableException instead of MalformedCarbonCommandException
|
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2583/ |
|
Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10843/ |
|
Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2814/ |
|
LGTM |
qiuchenjian
changed the title
|
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2599/ |
|
LGTM |
|
Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10858/ |
|
Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2829/ |
…e message of MalformedCarbonCommandException This closes #3106
…e message of MalformedCarbonCommandException This closes apache#3106
[Solution]
Compacting table that do not exist should throw NoSuchTableException instead of MalformedCarbonCommandException("Operation not allowed : ALTER TABLE table_name COMPACT 'MAJOR'")
it's confused
Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:
Any interfaces changed?
Any backward compatibility impacted?
Document update required?
Testing done
Please provide details on
- Whether new unit test cases have been added or why no new tests are required?
- How it is tested? Please attach test report.
- Is it a performance related change? Please attach the performance test report.
- Any additional information to help reviewers in testing this change.
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.