You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by traflm <gi...@git.apache.org> on 2016/04/19 05:35:27 UTC
[GitHub] incubator-trafodion pull request: [TRAFODION-1920] suppress SQL er...
GitHub user traflm opened a pull request:
https://github.com/apache/incubator-trafodion/pull/441
[TRAFODION-1920] suppress SQL error during HIVE_SCAN when encounter i…
…nvalid value, assign null to the invalid value
This pull request try to solve the issue when Trafodion try to retrieve data from a HDFS text file which contains invalid data. Like:
space for a numeric value
non-numeric string for a numeric value
this change will use the HIVE_SCAN_SPECAIL_MODE '2' , when that CQD is set to 2, HIVE_SCAN will automaticlly convert the invalid data into null and continue, instead of error out immediately.
How it does?
HIVE_SCAN need to do the data type conversion via expression evaluation. There are two ways to evaluate an expression: pCode and eval() function. Since the current HIVE_SCAN do conversion row by row, and details are encoded in corresponding pCode. So the simplest way I can find out is to fix the invalid data by set it to null and retry the whole evaluation.
In pCode and eval() function, right after a failure, it saves the offset of the offending source data pointer. In HIVE_SCAN work() function, once it got an evaluation error, find and fix the offending data in form of "R2" , exploded format, and retry the conversion. The maxium retry time is the number of columns.
So this change will only change pCode and normal evaluation a little bit. After an error, try to remember the offset of offending source data. It will not affect any normal evaluation/execution.
Only in HIVE_SCAN, in a special mode controlled by CQD, it will retry after the failure.
This will not introduce any unknown side effort.
It is easier and maybe faster to change pCode execution to set the target as null and continue, but since pCode is used in many cases, not only HIVE_SCAN move experession. So that change will introduce unknown side effect.
This retry logic does introduce overhead, but in normal case the code path is same as before, in special mode, if no invalid data, it will also go through same path as before, just retry in case there is converting error.
This change only fix from HIVE_SCAN perspective, to fully support transparent Bulkloader, we still need to change the bulkloader work() method to tolerate other source of failure. This change will make sure there will be no failure during HIVE_SCAN. Later, if there is CAST, or TRANSLATE or TRUNCATE issues, we still need to change bulkload code.
So this is the first part to fully support TRAFODION-1920, but it should handle a lot of real cases already.
Add some sample test cases in hive/TEST005 also, interesting reviewers can check the test for more details.
Please help to review, and this will be a help in some real use cases.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/traflm/incubator-trafodion TRAFODION-1912
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/441.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 #441
----
commit 8f78b446e9002ff9cd5e0f3ca2b5feead9726859
Author: Liu Ming <ov...@sina.com>
Date: 2016-04-19T03:02:16Z
[TRAFODION-1920] suppress SQL error during HIVE_SCAN when encounter invalid value, assign null to the invalid value
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1920] suppress SQL er...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/441#discussion_r60572080
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -1378,9 +1423,27 @@ ExWorkProcRetcode ExHdfsScanTcb::work()
return WORK_OK;
}
+void ExHdfsScanTcb::setR2ColumnNull(Int32 colidx)
+{
+ Lng32 neededColIndex = 0;
+ Attributes * attr = NULL;
+ ExpTupleDesc * asciiSourceTD = hdfsScanTdb().workCriDesc_->getTupleDescriptor(hdfsScanTdb().asciiTuppIndex_);
+ for (Lng32 i = 0; i < hdfsScanTdb().convertSkipListSize_; i++)
+ {
+ if (hdfsScanTdb().convertSkipList_[i] > 0)
+ {
+ attr = asciiSourceTD->getAttr(neededColIndex);
+ neededColIndex++;
+ if(attr->getOffset() == colidx)
+ {
+ *(short *)&hdfsAsciiSourceData_[attr->getNullIndOffset()] = -1;
+ }
+ }
+ }
+}
--- End diff --
Thanks Selva, I agree, will change it as well. It will support more converting cases. If simply select * from hive, convertExpr() will be null. So I ignore it. But if one add some cast in the select list, it will be invoked, so same logic should apply to it as well.
I will have an update asap.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1920] suppress SQL er...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/441#discussion_r60527463
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -1378,9 +1423,27 @@ ExWorkProcRetcode ExHdfsScanTcb::work()
return WORK_OK;
}
+void ExHdfsScanTcb::setR2ColumnNull(Int32 colidx)
+{
+ Lng32 neededColIndex = 0;
+ Attributes * attr = NULL;
+ ExpTupleDesc * asciiSourceTD = hdfsScanTdb().workCriDesc_->getTupleDescriptor(hdfsScanTdb().asciiTuppIndex_);
+ for (Lng32 i = 0; i < hdfsScanTdb().convertSkipListSize_; i++)
+ {
+ if (hdfsScanTdb().convertSkipList_[i] > 0)
+ {
+ attr = asciiSourceTD->getAttr(neededColIndex);
+ neededColIndex++;
+ if(attr->getOffset() == colidx)
+ {
+ *(short *)&hdfsAsciiSourceData_[attr->getNullIndOffset()] = -1;
+ }
+ }
+ }
+}
--- End diff --
I think both convertExpr() and moveColsConvertExpr() deals at a row level.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1920] suppress SQL er...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/441#discussion_r60329781
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -1378,9 +1423,27 @@ ExWorkProcRetcode ExHdfsScanTcb::work()
return WORK_OK;
}
+void ExHdfsScanTcb::setR2ColumnNull(Int32 colidx)
+{
+ Lng32 neededColIndex = 0;
+ Attributes * attr = NULL;
+ ExpTupleDesc * asciiSourceTD = hdfsScanTdb().workCriDesc_->getTupleDescriptor(hdfsScanTdb().asciiTuppIndex_);
+ for (Lng32 i = 0; i < hdfsScanTdb().convertSkipListSize_; i++)
+ {
+ if (hdfsScanTdb().convertSkipList_[i] > 0)
+ {
+ attr = asciiSourceTD->getAttr(neededColIndex);
+ neededColIndex++;
+ if(attr->getOffset() == colidx)
+ {
+ *(short *)&hdfsAsciiSourceData_[attr->getNullIndOffset()] = -1;
+ }
+ }
+ }
+}
--- End diff --
I'm still reviewing, and I'm not familiar with this code. I'm trying to understand how this works. Does hdfsAsciiSourceData_ point to an ASCII string? Why would putting two bytes of x'ff' into the string make a value look like a null? How does the move expression parse to the next field?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1920] suppress SQL er...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/441#discussion_r60524880
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -1378,9 +1423,27 @@ ExWorkProcRetcode ExHdfsScanTcb::work()
return WORK_OK;
}
+void ExHdfsScanTcb::setR2ColumnNull(Int32 colidx)
+{
+ Lng32 neededColIndex = 0;
+ Attributes * attr = NULL;
+ ExpTupleDesc * asciiSourceTD = hdfsScanTdb().workCriDesc_->getTupleDescriptor(hdfsScanTdb().asciiTuppIndex_);
+ for (Lng32 i = 0; i < hdfsScanTdb().convertSkipListSize_; i++)
+ {
+ if (hdfsScanTdb().convertSkipList_[i] > 0)
+ {
+ attr = asciiSourceTD->getAttr(neededColIndex);
+ neededColIndex++;
+ if(attr->getOffset() == colidx)
+ {
+ *(short *)&hdfsAsciiSourceData_[attr->getNullIndOffset()] = -1;
+ }
+ }
+ }
+}
--- End diff --
I wonder if we need to change it to NULL at line no 1585 when convertExpr() returns an error if the column is nullable and the cqd is set to convert to null. It is not clear to me the need for other changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1920] suppress SQL er...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/441#discussion_r60336260
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -1378,9 +1423,27 @@ ExWorkProcRetcode ExHdfsScanTcb::work()
return WORK_OK;
}
+void ExHdfsScanTcb::setR2ColumnNull(Int32 colidx)
+{
+ Lng32 neededColIndex = 0;
+ Attributes * attr = NULL;
+ ExpTupleDesc * asciiSourceTD = hdfsScanTdb().workCriDesc_->getTupleDescriptor(hdfsScanTdb().asciiTuppIndex_);
+ for (Lng32 i = 0; i < hdfsScanTdb().convertSkipListSize_; i++)
+ {
+ if (hdfsScanTdb().convertSkipList_[i] > 0)
+ {
+ attr = asciiSourceTD->getAttr(neededColIndex);
+ neededColIndex++;
+ if(attr->getOffset() == colidx)
+ {
+ *(short *)&hdfsAsciiSourceData_[attr->getNullIndOffset()] = -1;
+ }
+ }
+ }
+}
--- End diff --
thanks Dave,
Please refer to the very excellent comments above src code in FileScan::codeGenForHive().
The hdfsAsciiSourceData_ is the pointer to row 'R2' referred to in those comments. It is in a special internal tuple format called 'exploded format row', each field in this tuple contains at least two parts as far as I know: first byte is null-indicator, when it is 0, means not null, when it is 0xFF (-1), it means null. After the one byte null indicator, it follows 8 bytes of value, but it is not the real value, just a pointer to R1. When need, it needs to deference the pointer. R1 is a buffer containing raw data read from HDFS file. R3 is for predicate, R4 is for projection and return rows.
So set 0xff here will set required field in R2 as null, in this case, it is because it contains invalid data, like 'abc' for an Integer field. And the moveColsConvertExpr expression failed to convert it into an integer. So the logic go back here , calling this method to set the invalid data field to null. And try again. Because the evaluation is now done row by row, not column by column, so this is the simplest way to change the logic at present . There are better ways, but without more guidance, I cannot do those, for example: do this converting column by column, this requires a huge change in compiler to generate different pCode and expression tree, which beyond my knowledge.
I didn't change any current normal logic.
The move expression typically will generate a piece of pCode instructions, if the row has 3 columns, the generated pCode will have 3 sets of instructions like this(instruction name is not valid, just try to explain what I want to mean)
IFNULL
INST1
ELSE
INST2
I modified INST2 to save the offset of invalid data offset in 'R2', and everything else keep same.
If pCode is turned off, the expression will be evaluated in normal clause evaluation path. It is a list of clauses, each clause correspond to an operation, and each column has a convert clause to do the data converting if needed. This is generated inside compiler, I don't know the details, parser should generate a tree of ItemExpr, and gencode will generate correct expression.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---