[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3817: [CARBONDATA-3845] Bucket table creation fails with exception for empt…
Indhumathi27 commented on a change in pull request #3817:
URL: https://github.com/apache/carbondata/pull/3817#discussion_r449371217
##
File path:
integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##
@@ -766,13 +766,13 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
throw new MalformedCarbonCommandException("Invalid table properties")
}
if (options.isBucketingEnabled) {
- if (options.bucketNumber.toString.contains("-") ||
- options.bucketNumber.toString.contains("+") || options.bucketNumber
== 0) {
+ if (options.bucketNumber.isEmpty ||
options.bucketNumber.get.toString.contains("-")
+|| options.bucketNumber.get == 0) {
Review comment:
Please format these two lines
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3817: [CARBONDATA-3845] Bucket table creation fails with exception for empt…
Indhumathi27 commented on a change in pull request #3817:
URL: https://github.com/apache/carbondata/pull/3817#discussion_r448997627
##
File path:
integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##
@@ -766,13 +766,13 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
throw new MalformedCarbonCommandException("Invalid table properties")
}
if (options.isBucketingEnabled) {
- if (options.bucketNumber.toString.contains("-") ||
- options.bucketNumber.toString.contains("+") || options.bucketNumber
== 0) {
+ if (options.bucketNumber.isEmpty ||
options.bucketNumber.get.toString.contains("-") ||
Review comment:
Since you have wrapped options with Try, i guess in case if Bucket
number as "+" and "-", it will be empty. Can check and remove those checks
##
File path:
integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##
@@ -766,13 +766,13 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
throw new MalformedCarbonCommandException("Invalid table properties")
}
if (options.isBucketingEnabled) {
- if (options.bucketNumber.toString.contains("-") ||
- options.bucketNumber.toString.contains("+") || options.bucketNumber
== 0) {
+ if (options.bucketNumber.isEmpty ||
options.bucketNumber.get.toString.contains("-") ||
Review comment:
Since you have wrapped bucket options with Try, i guess in case if
Bucket number as "+" and "-", it will be empty. Can check and remove those
checks
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3817: [CARBONDATA-3845] Bucket table creation fails with exception for empt…
Indhumathi27 commented on a change in pull request #3817:
URL: https://github.com/apache/carbondata/pull/3817#discussion_r448797700
##
File path:
integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##
@@ -766,13 +766,13 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
throw new MalformedCarbonCommandException("Invalid table properties")
}
if (options.isBucketingEnabled) {
- if (options.bucketNumber.toString.contains("-") ||
- options.bucketNumber.toString.contains("+") || options.bucketNumber
== 0) {
+ if (options.bucketNumber == None ||
options.bucketNumber.get.toString.contains("-") ||
Review comment:
```suggestion
if (options.bucketNumber.isEmpty ||
options.bucketNumber.get.toString.contains("-") ||
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3817: [CARBONDATA-3845] Bucket table creation fails with exception for empt…
Indhumathi27 commented on a change in pull request #3817: URL: https://github.com/apache/carbondata/pull/3817#discussion_r448797371 ## File path: integration/spark/src/main/scala/org/apache/carbondata/spark/CarbonOption.scala ## @@ -17,6 +17,8 @@ package org.apache.carbondata.spark +import scala.util.Try + Review comment: Please remove extra line This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3817: [CARBONDATA-3845] Bucket table creation fails with exception for empt…
Indhumathi27 commented on a change in pull request #3817:
URL: https://github.com/apache/carbondata/pull/3817#discussion_r448249998
##
File path:
integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##
@@ -766,13 +766,13 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
throw new MalformedCarbonCommandException("Invalid table properties")
}
if (options.isBucketingEnabled) {
- if (options.bucketNumber.toString.contains("-") ||
- options.bucketNumber.toString.contains("+") || options.bucketNumber
== 0) {
+ if (options.bucketNumber == None ||
options.bucketNumber.get.toString.contains("-") ||
+ options.bucketNumber.get.toString.contains("+") ||
options.bucketNumber.get == 0) {
Review comment:
Please add testcases for these scenarios
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
