GitHub user mohammadshahidkhan opened a pull request:
https://github.com/apache/carbondata/pull/2104 [CARBONDATA-2285] Spark integration code refactor Be sure to do all of the following checklist to help us incorporate your contribution quickly and easily: - [X] Any interfaces changed? ICarbonSessionCatalog added to to unify the sessioncataloag usages across spark-2.1 & spark-2.2 interation. - [X] Any backward compatibility impactedX None - [X] Document update required? None - [X] 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. Not new test case added as there is no functional modification. - [X] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA. NA You can merge this pull request into a Git repository by running: $ git pull https://github.com/mohammadshahidkhan/incubator-carbondata code_refactor Alternatively you can review and apply these changes as the patch at: https://github.com/apache/carbondata/pull/2104.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2104 ---- commit 1cbc57c8e6a250f2d70f47dd1d5202db2454c77d Author: mohammadshahidkhan <mohdshahidkhan1987@...> Date: 2018-03-26T11:06:29Z [CARBONDATA-2285] Spark integration code refactor ---- --- |
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2104 Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/3373/ --- |
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2104 Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/4599/ --- |
In reply to this post by qiuchenjian-2
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2104 SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4098/ --- |
In reply to this post by qiuchenjian-2
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r177136267 --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java --- @@ -1297,6 +1297,12 @@ public static final String CARBON_SESSIONSTATE_CLASSNAME = "spark.carbon.sessionstate.classname"; + /** + * This property will be used to configure the sqlastbuilder class. --- End diff -- Why sqlastbuilder class configuration is required? --- |
In reply to this post by qiuchenjian-2
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r177136802 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/ICarbonSessionCatalog.scala --- @@ -0,0 +1,59 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +package org.apache.spark.sql.hive + +import org.apache.hadoop.fs.Path +import org.apache.spark.sql.{CarbonEnv, SparkSession} +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTablePartition} +import org.apache.spark.sql.catalyst.expressions.Expression + +/** + * trait to give contract to the CarbonSessionCatalog + */ +trait ICarbonSessionCatalog { --- End diff -- annotate it with InterfaceAudience and InterfaceStability --- |
In reply to this post by qiuchenjian-2
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r177137010 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/ICarbonSessionCatalog.scala --- @@ -0,0 +1,59 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +package org.apache.spark.sql.hive + +import org.apache.hadoop.fs.Path +import org.apache.spark.sql.{CarbonEnv, SparkSession} +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTablePartition} +import org.apache.spark.sql.catalyst.expressions.Expression + +/** + * trait to give contract to the CarbonSessionCatalog + */ +trait ICarbonSessionCatalog { --- End diff -- rename to `CarbonSessionCatalog` and give more description in comment --- |
In reply to this post by qiuchenjian-2
Github user mohammadshahidkhan commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r177416365 --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java --- @@ -1297,6 +1297,12 @@ public static final String CARBON_SESSIONSTATE_CLASSNAME = "spark.carbon.sessionstate.classname"; + /** + * This property will be used to configure the sqlastbuilder class. --- End diff -- This required for decouple feature. --- |
In reply to this post by qiuchenjian-2
Github user mohammadshahidkhan commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r177423796 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/ICarbonSessionCatalog.scala --- @@ -0,0 +1,59 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +package org.apache.spark.sql.hive + +import org.apache.hadoop.fs.Path +import org.apache.spark.sql.{CarbonEnv, SparkSession} +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTablePartition} +import org.apache.spark.sql.catalyst.expressions.Expression + +/** + * trait to give contract to the CarbonSessionCatalog + */ +trait ICarbonSessionCatalog { --- End diff -- It could not be renamed to CarbonSessionCatalog as it already exists. Added comments --- |
In reply to this post by qiuchenjian-2
Github user mohammadshahidkhan commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r177426177 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/ICarbonSessionCatalog.scala --- @@ -0,0 +1,59 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +package org.apache.spark.sql.hive + +import org.apache.hadoop.fs.Path +import org.apache.spark.sql.{CarbonEnv, SparkSession} +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTablePartition} +import org.apache.spark.sql.catalyst.expressions.Expression + +/** + * trait to give contract to the CarbonSessionCatalog + */ +trait ICarbonSessionCatalog { --- End diff -- Added anotation --- |
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2104 Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/3396/ --- |
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2104 Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/4621/ --- |
In reply to this post by qiuchenjian-2
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2104 SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4124/ --- |
In reply to this post by qiuchenjian-2
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r178030727 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/ICarbonSessionCatalog.scala --- @@ -0,0 +1,67 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +package org.apache.spark.sql.hive + +import org.apache.hadoop.fs.Path +import org.apache.spark.sql.{CarbonEnv, SparkSession} +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTablePartition} +import org.apache.spark.sql.catalyst.expressions.Expression + +import org.apache.carbondata.common.annotations.{InterfaceAudience, InterfaceStability} + +/** + * This interface defines those common api used by carbon for spark-2.1 and spark-2.2 integration, + * but are not defined in SessionCatalog or HiveSessionCatalog to give contract to the + * Concrete implementation classes. + * For example CarbonSessionCatalog defined in 2.1 and 2.2. + * + */ +@InterfaceAudience.Internal +@InterfaceStability.Stable +trait ICarbonSessionCatalog { --- End diff -- I think the best approach to verify whether it is really abstracted is to verify it with spark-2.3, can you check once by adding profile for spark-2.3 and report in this PR --- |
In reply to this post by qiuchenjian-2
Github user mohammadshahidkhan commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2104#discussion_r178051841 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/ICarbonSessionCatalog.scala --- @@ -0,0 +1,67 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. See the NOTICE file distributed with +* this work for additional information regarding copyright ownership. +* The ASF licenses this file to You under the Apache License, Version 2.0 +* (the "License"); you may not use this file except in compliance with +* the License. You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +package org.apache.spark.sql.hive + +import org.apache.hadoop.fs.Path +import org.apache.spark.sql.{CarbonEnv, SparkSession} +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTablePartition} +import org.apache.spark.sql.catalyst.expressions.Expression + +import org.apache.carbondata.common.annotations.{InterfaceAudience, InterfaceStability} + +/** + * This interface defines those common api used by carbon for spark-2.1 and spark-2.2 integration, + * but are not defined in SessionCatalog or HiveSessionCatalog to give contract to the + * Concrete implementation classes. + * For example CarbonSessionCatalog defined in 2.1 and 2.2. + * + */ +@InterfaceAudience.Internal +@InterfaceStability.Stable +trait ICarbonSessionCatalog { --- End diff -- Fixed renames ICarbonSessionCatalog to CarbonSessionCatalog and CarbonSessionCatalog to ICarbonHiveSessionCatalog --- |
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2104 Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/4664/ --- |
In reply to this post by qiuchenjian-2
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2104 Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/3438/ --- |
In reply to this post by qiuchenjian-2
|
In reply to this post by qiuchenjian-2
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2104 SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4176/ --- |
Free forum by Nabble | Edit this page |