[CARBONDATA-3562] Fix for SDK filter queries not working when schema is given explicitly while Add Segment #3427
Conversation
|
@ravipesala Please review. |
|
Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/717/ |
manishnalla1994
force-pushed
the
SDKIssueFix
branch
from
2bd74bc
to
e3dab13
Compare
|
Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/718/ |
|
Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/725/ |
|
Build Failed with Spark 2.3.2, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/726/ |
| @@ -722,12 +723,28 @@ class AddSegmentTestCase extends QueryTest with BeforeAndAfterAll { | |||
| val externalSegmentPath = storeLocation + "/" + "external_segment" | |||
| FileFactory.deleteAllFilesOfDir(new File(externalSegmentPath)) | |||
|
|
|||
| var fields: Array[Field] = new Array[Field](14) | |||
There was a problem hiding this comment.
why is this test case changed?
There was a problem hiding this comment.
This test is changed just to check for the schema which we give externally instead of referring to schema file of the already existing table.
|
@manishnalla1994 I think this fix might induce new issues in alter table , like drop column and add back the column with the same name might not work as expected. |
|
@ravipesala when we add the same column then the column id will be different, so it wont match in our case. |
manishnalla1994
force-pushed
the
SDKIssueFix
branch
2 times, most recently
from
318a118
to
fbb55fb
Compare
|
Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/745/ |
|
Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/751/ |
| @@ -167,15 +167,22 @@ public static boolean isColumnMatches(boolean isTransactionalTable, | |||
| // column ID but can have same column name | |||
| if (tableColumn.getDataType().isComplexType() && !(tableColumn.getDataType().getId() | |||
| == DataTypes.ARRAY_TYPE_ID)) { | |||
| if (tableColumn.getColumnId().equalsIgnoreCase(queryColumn.getColumnId())) { | |||
| if (tableColumn.getColumnId().equalsIgnoreCase(queryColumn.getColumnId()) || ( | |||
There was a problem hiding this comment.
Can you override equals method of Column or add method inside a column to do this check. It is pretty repetitive and more prone to issues.
| if (carbonDimension.getColumnSchema().getColumnUniqueId() | ||
| .equalsIgnoreCase(queryColumn.getColumnId())) { | ||
| .equalsIgnoreCase(queryColumn.getColumnId()) || ( | ||
| carbonDimension.getColumnSchema().getColumnUniqueId() |
There was a problem hiding this comment.
Why there is a diff of condition check for dimension and measure? Here we compare with ColumnUniqueId but other places are not
There was a problem hiding this comment.
This is only for the case of struct matching, as was before, just added one more check.
|
Build Success with Spark 2.3.2, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/753/ |
manishnalla1994
force-pushed
the
SDKIssueFix
branch
from
fbb55fb
to
d92db6f
Compare
|
Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/754/ |
|
Build Success with Spark 2.3.2, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/762/ |
|
Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/760/ |
|
LGTM |
1 similar comment
|
LGTM |
|
retest this please |
|
Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/784/ |
|
Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/790/ |
|
Build Success with Spark 2.3.2, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/792/ |
manishnalla1994
force-pushed
the
SDKIssueFix
branch
from
d92db6f
to
fb6ca4d
Compare
manishnalla1994
force-pushed
the
SDKIssueFix
branch
from
fb6ca4d
to
80277f0
Compare
|
Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/789/ |
|
Build Success with Spark 2.3.2, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/797/ |
|
Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/795/ |
|
LGTM |
Problem1 : Queries will not return correct result from added segment when the schema is given explicitly in case of SDK.
Solution : Handled it by validating based on both column name and column id if it matches for the SDK column.
Problem2 : While deleting added segment, the physical location is also getting deleted.
Solution: Fixed that by adding validation.
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.