You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by sounakr <gi...@git.apache.org> on 2018/05/02 03:32:40 UTC
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
GitHub user sounakr opened a pull request:
https://github.com/apache/carbondata/pull/2257
[CARBONDATA-2423][SDK]SDK Reader support to read from Non Transactional Table
SDK Reader support to read from Non Transactional Table
- [ ] 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sounakr/incubator-carbondata reader
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/carbondata/pull/2257.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 #2257
----
commit 3cde4cc26bad63a2370609426778e9716a3733cb
Author: sounakr <so...@...>
Date: 2018-05-02T03:26:09Z
CarbonData Reader Support For Non Transactional Table
commit dae5a508cf116db1dc1eee24063a5f80680e816d
Author: sounakr <so...@...>
Date: 2018-05-02T03:30:37Z
Review
----
---
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2257#discussion_r185400612
--- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
@@ -244,10 +244,17 @@ public static CarbonTable buildFromDataFile(
return buildFromTableInfo(tableInfo);
}
- public static CarbonTable buildFromTablePath(
- String tableName, String tablePath) throws IOException {
- return SchemaReader.readCarbonTableFromStore(
- AbsoluteTableIdentifier.from(tablePath, tableName, "default"));
+ public static CarbonTable buildFromTablePath(String tableName, String tablePath,
+ boolean isTransactionalTable) throws IOException {
+ if (isTransactionalTable) {
+ return SchemaReader
+ .readCarbonTableFromStore(AbsoluteTableIdentifier.from(tablePath, tableName, "default"));
--- End diff --
The arguments are reversed, seconf argument is dbname not tablename. And pass proper dbname from arguments
---
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2257#discussion_r185400031
--- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
@@ -244,10 +244,17 @@ public static CarbonTable buildFromDataFile(
return buildFromTableInfo(tableInfo);
}
- public static CarbonTable buildFromTablePath(
- String tableName, String tablePath) throws IOException {
- return SchemaReader.readCarbonTableFromStore(
- AbsoluteTableIdentifier.from(tablePath, tableName, "default"));
+ public static CarbonTable buildFromTablePath(String tableName, String tablePath,
+ boolean isTransactionalTable) throws IOException {
--- End diff --
I don't think it is required to pass `isTransactionalTable` to this mehod, handle inside `SchemaReader.readCarbonTableFromStore` if there is no schema exists then go for inferring.
---
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
Posted by sounakr <gi...@git.apache.org>.
Github user sounakr commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2257#discussion_r185403178
--- Diff: hadoop/src/main/java/org/apache/carbondata/hadoop/api/CarbonFileInputFormat.java ---
@@ -126,13 +132,27 @@ protected CarbonTable getOrCreateCarbonTable(Configuration configuration) throws
FilterResolverIntf filterInterface = carbonTable.resolveFilter(filter, tableProvider);
- String segmentDir = CarbonTablePath.getSegmentPath(identifier.getTablePath(), "null");
+ String segmentDir = null;
+ if (carbonTable.isTransactionalTable()) {
+ segmentDir = CarbonTablePath.getSegmentPath(identifier.getTablePath(), "null");
+ } else {
+ segmentDir = identifier.getTablePath();
+ }
FileFactory.FileType fileType = FileFactory.getFileType(segmentDir);
if (FileFactory.isFileExist(segmentDir, fileType)) {
// if external table Segments are found, add it to the List
List<Segment> externalTableSegments = new ArrayList<Segment>();
- Segment seg = new Segment("null", null, readCommittedScope);
- externalTableSegments.add(seg);
+ Segment seg;
+ if (carbonTable.isTransactionalTable()) {
+ seg = new Segment("null", null, readCommittedScope);
--- End diff --
Prior Non Transactional implementation, SDK used to write into the Segment Path instead of Table Path. i.e. inside /Fact/Part0/Sement_null. It used to deliberately pass the segment as "null". Those codes to read and write into the segment path is still existing and they make it an external transactional table and the code flow goes through CarbonInputFormat.
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2257
Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4416/
---
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/carbondata/pull/2257
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2257
LGTM
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2257
Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4407/
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on the issue:
https://github.com/apache/carbondata/pull/2257
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4671/
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2257
Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4410/
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2257
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5569/
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2257
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5571/
---
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
Posted by sounakr <gi...@git.apache.org>.
Github user sounakr commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2257#discussion_r185401794
--- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
@@ -244,10 +244,17 @@ public static CarbonTable buildFromDataFile(
return buildFromTableInfo(tableInfo);
}
- public static CarbonTable buildFromTablePath(
- String tableName, String tablePath) throws IOException {
- return SchemaReader.readCarbonTableFromStore(
- AbsoluteTableIdentifier.from(tablePath, tableName, "default"));
+ public static CarbonTable buildFromTablePath(String tableName, String tablePath,
+ boolean isTransactionalTable) throws IOException {
+ if (isTransactionalTable) {
+ return SchemaReader
+ .readCarbonTableFromStore(AbsoluteTableIdentifier.from(tablePath, tableName, "default"));
--- End diff --
Done.
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by CarbonDataQA <gi...@git.apache.org>.
Github user CarbonDataQA commented on the issue:
https://github.com/apache/carbondata/pull/2257
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5577/
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by sounakr <gi...@git.apache.org>.
Github user sounakr commented on the issue:
https://github.com/apache/carbondata/pull/2257
Retest this please
---
[GitHub] carbondata issue #2257: [CARBONDATA-2423][SDK]SDK Reader support to read fro...
Posted by sounakr <gi...@git.apache.org>.
Github user sounakr commented on the issue:
https://github.com/apache/carbondata/pull/2257
Retest this please
---
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
Posted by sounakr <gi...@git.apache.org>.
Github user sounakr commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2257#discussion_r185402417
--- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
@@ -244,10 +244,17 @@ public static CarbonTable buildFromDataFile(
return buildFromTableInfo(tableInfo);
}
- public static CarbonTable buildFromTablePath(
- String tableName, String tablePath) throws IOException {
- return SchemaReader.readCarbonTableFromStore(
- AbsoluteTableIdentifier.from(tablePath, tableName, "default"));
+ public static CarbonTable buildFromTablePath(String tableName, String tablePath,
+ boolean isTransactionalTable) throws IOException {
--- End diff --
From the reader perspective wanted to make a clear distinction if it is reading a Transactional or Non Transactional table. So let's not infer in case of transactional table. There may be some corruption due to which schema might get deleted in transactional table but we are able to infer schema and move forward, in this scenario user wont be able to know about the corruption present.
---
[GitHub] carbondata pull request #2257: [CARBONDATA-2423][SDK]SDK Reader support to r...
Posted by ravipesala <gi...@git.apache.org>.
Github user ravipesala commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2257#discussion_r185401415
--- Diff: hadoop/src/main/java/org/apache/carbondata/hadoop/api/CarbonFileInputFormat.java ---
@@ -126,13 +132,27 @@ protected CarbonTable getOrCreateCarbonTable(Configuration configuration) throws
FilterResolverIntf filterInterface = carbonTable.resolveFilter(filter, tableProvider);
- String segmentDir = CarbonTablePath.getSegmentPath(identifier.getTablePath(), "null");
+ String segmentDir = null;
+ if (carbonTable.isTransactionalTable()) {
+ segmentDir = CarbonTablePath.getSegmentPath(identifier.getTablePath(), "null");
+ } else {
+ segmentDir = identifier.getTablePath();
+ }
FileFactory.FileType fileType = FileFactory.getFileType(segmentDir);
if (FileFactory.isFileExist(segmentDir, fileType)) {
// if external table Segments are found, add it to the List
List<Segment> externalTableSegments = new ArrayList<Segment>();
- Segment seg = new Segment("null", null, readCommittedScope);
- externalTableSegments.add(seg);
+ Segment seg;
+ if (carbonTable.isTransactionalTable()) {
+ seg = new Segment("null", null, readCommittedScope);
--- End diff --
this `null` segment and transactional is very confusing, what is the use of those?
---