You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by st...@apache.org on 2022/12/06 04:27:39 UTC
[impala] 01/02: IMPALA-11696: Fix incorrect warnings of ignoring delimiters on text/sequence tables
This is an automated email from the ASF dual-hosted git repository.
stigahuang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 6838e988a06250ee22a78d684990bd8e88ab46a0
Author: stiga-huang <hu...@gmail.com>
AuthorDate: Wed Nov 2 11:02:00 2022 +0800
IMPALA-11696: Fix incorrect warnings of ignoring delimiters on text/sequence tables
IMPALA-9822 adds a warning when the customized row format delimiters in
the CreateTable statement are ignored on non-TEXT and non-SEQUENCE
tables. However, the warning also shows up for TEXT/SEQUENCE tables. The
cause is an incorrect check in the table format that all formats match
the condition.
This patch fixes the condition and adds tests to verify that no warnings
show up in such cases. Currently the test methods (e.g. AnalyzesOk) only
check expected warning messages when provided. If the provided expected
message is null, they just skip checking the warnings. This patch adds
methods like AnalyzesOkWithoutWarnings to assure no warnings are
generated.
Tests
- Run FE tests
Change-Id: I0871b94dcd2290723699c21227a576e8a6a09b5a
Reviewed-on: http://gerrit.cloudera.org:8080/19186
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../apache/impala/analysis/CreateTableStmt.java | 2 +-
.../org/apache/impala/analysis/AnalyzeDDLTest.java | 37 +++++++++++++---------
.../apache/impala/analysis/AnalyzeStmtsTest.java | 6 +++-
.../org/apache/impala/common/FrontendFixture.java | 10 ++++--
.../org/apache/impala/common/FrontendTestBase.java | 24 ++++++++++++--
5 files changed, 56 insertions(+), 23 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java b/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
index f2452aff8..0c36b8eb9 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
@@ -277,7 +277,7 @@ public class CreateTableStmt extends StatementBase {
String lineDelimiter = getRowFormat().getLineDelimiter();
String escapeChar = getRowFormat().getEscapeChar();
if (getFileFormat() != THdfsFileFormat.TEXT
- || getFileFormat() != THdfsFileFormat.SEQUENCE_FILE) {
+ && getFileFormat() != THdfsFileFormat.SEQUENCE_FILE) {
if (fieldDelimiter != null) {
analyzer.addWarning("'ROW FORMAT DELIMITED FIELDS TERMINATED BY '"
+ fieldDelimiter + "'' is ignored.");
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
index 30d02eff6..457e2dded 100755
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
@@ -49,7 +49,6 @@ import org.apache.impala.common.FileSystemUtil;
import org.apache.impala.common.FrontendTestBase;
import org.apache.impala.common.Pair;
import org.apache.impala.common.PrintUtils;
-import org.apache.impala.common.RuntimeEnv;
import org.apache.impala.service.BackendConfig;
import org.apache.impala.testutil.TestUtils;
import org.apache.impala.thrift.TBackendGflags;
@@ -2369,16 +2368,20 @@ public class AnalyzeDDLTest extends FrontendTestBase {
"from functional.alltypestiny");
// IMPALA-9822 Row Format Delimited is valid only for Text Files
- String[] fileFormats = {"PARQUET", "ICEBERG"};
- for (String format : fileFormats) {
+ String[] fileFormats = {"TEXTFILE", "PARQUET", "ICEBERG"};
+ for (int i = 0; i < fileFormats.length; ++i) {
+ String format = fileFormats[i];
for (String rowFormat : ImmutableList.of(
"FIELDS TERMINATED BY ','", "LINES TERMINATED BY ','", "ESCAPED BY ','")) {
- AnalyzesOk(
- String.format(
- "create table new_table row format delimited %s stored as %s as select *"
- + " from functional.child_table",
- rowFormat, format),
- "'ROW FORMAT DELIMITED " + rowFormat + "' is ignored.");
+ String stmt = String.format(
+ "create table new_table row format delimited %s stored as %s as select *"
+ + " from functional.child_table", rowFormat, format);
+ if (i == 0) {
+ // No warrnings for TEXT tables
+ AnalyzesOkWithoutWarnings(stmt);
+ } else {
+ AnalyzesOk(stmt, "'ROW FORMAT DELIMITED " + rowFormat + "' is ignored.");
+ }
}
}
}
@@ -2638,14 +2641,18 @@ public class AnalyzeDDLTest extends FrontendTestBase {
formatIndx++;
}
- for (formatIndx = 2; formatIndx < fileFormats.length; formatIndx++) {
+ for (formatIndx = 0; formatIndx < fileFormats.length; formatIndx++) {
for (String rowFormat : ImmutableList.of(
"FIELDS TERMINATED BY ','", "LINES TERMINATED BY ','", "ESCAPED BY ','")) {
- AnalyzesOk(
- String.format(
- "create table new_table (i int) row format delimited %s stored as %s",
- rowFormat, fileFormats[formatIndx]),
- "'ROW FORMAT DELIMITED " + rowFormat + "' is ignored");
+ String stmt = String.format(
+ "create table new_table (i int) row format delimited %s stored as %s",
+ rowFormat, fileFormats[formatIndx]);
+ if (formatIndx < 2) {
+ // No warrnings for TEXT and SEQUENCE tables
+ AnalyzesOkWithoutWarnings(stmt);
+ } else {
+ AnalyzesOk(stmt, "'ROW FORMAT DELIMITED " + rowFormat + "' is ignored");
+ }
}
}
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
index 7c83f5c19..9fbbd1d72 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
@@ -2028,7 +2028,11 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
*/
private boolean hasStraightJoin(String stmt, String expectedWarning){
AnalysisContext ctx = createAnalysisCtx();
- AnalyzesOk(stmt,ctx, expectedWarning);
+ if (expectedWarning == null) {
+ AnalyzesOkWithoutWarnings(stmt, ctx);
+ } else {
+ AnalyzesOk(stmt, ctx, expectedWarning);
+ }
return ctx.getAnalyzer().isStraightJoin();
}
diff --git a/fe/src/test/java/org/apache/impala/common/FrontendFixture.java b/fe/src/test/java/org/apache/impala/common/FrontendFixture.java
index 862b782e1..17e9d6665 100644
--- a/fe/src/test/java/org/apache/impala/common/FrontendFixture.java
+++ b/fe/src/test/java/org/apache/impala/common/FrontendFixture.java
@@ -373,13 +373,14 @@ public class FrontendFixture {
/**
* Analyze 'stmt', expecting it to pass. Asserts in case of analysis error.
* If 'expectedWarning' is not null, asserts that a warning is produced.
+ * Otherwise, asserts no warnings if 'assertNoWarnings' is true.
*/
public ParseNode analyzeStmt(String stmt, AnalysisContext ctx,
- String expectedWarning) {
+ String expectedWarning, boolean assertNoWarnings) {
try {
AnalysisResult analysisResult = parseAndAnalyze(stmt, ctx);
+ List<String> actualWarnings = analysisResult.getAnalyzer().getWarnings();
if (expectedWarning != null) {
- List<String> actualWarnings = analysisResult.getAnalyzer().getWarnings();
boolean matchedWarning = false;
for (String actualWarning: actualWarnings) {
if (actualWarning.startsWith(expectedWarning)) {
@@ -392,6 +393,9 @@ public class FrontendFixture {
+ "Expected warning:\n%s.\nActual warnings:\n%s\nsql:\n%s",
expectedWarning, Joiner.on("\n").join(actualWarnings), stmt));
}
+ } else if (assertNoWarnings && !actualWarnings.isEmpty()) {
+ fail(String.format("Should not produce any warnings. Got:\n%s\nsql:\n%s",
+ Joiner.on("\n").join(actualWarnings), stmt));
}
Preconditions.checkNotNull(analysisResult.getStmt());
return analysisResult.getStmt();
@@ -407,6 +411,6 @@ public class FrontendFixture {
* Uses default options; use {@link QueryFixture} for greater control.
*/
public ParseNode analyzeStmt(String stmt) {
- return analyzeStmt(stmt, createAnalysisCtx(), null);
+ return analyzeStmt(stmt, createAnalysisCtx(), null, false);
}
}
diff --git a/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java b/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
index 4bf1af1a6..f549c61f9 100644
--- a/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
+++ b/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
@@ -196,9 +196,17 @@ public class FrontendTestBase extends AbstractFrontendTest {
/**
* Analyze 'stmt', expecting it to pass. Asserts in case of analysis error.
* If 'expectedWarning' is not null, asserts that a warning is produced.
+ * Otherwise, asserts no warnings.
*/
public ParseNode AnalyzesOk(String stmt, String expectedWarning) {
- return AnalyzesOk(stmt, createAnalysisCtx(), expectedWarning);
+ return AnalyzesOk(stmt, createAnalysisCtx(), expectedWarning, true);
+ }
+
+ /**
+ * Analyze 'stmt', expecting it to pass. Asserts in case of analysis error or warnings.
+ */
+ public ParseNode AnalyzesOkWithoutWarnings(String stmt) {
+ return AnalyzesOk(stmt, createAnalysisCtx(), null, true);
}
protected AnalysisContext createAnalysisCtx() {
@@ -243,13 +251,23 @@ public class FrontendTestBase extends AbstractFrontendTest {
/**
* Analyze 'stmt', expecting it to pass. Asserts in case of analysis error.
* If 'expectedWarning' is not null, asserts that a warning is produced.
+ * Otherwise, asserts no warnings if 'assertNoWarnings' is true.
*/
- public ParseNode AnalyzesOk(String stmt, AnalysisContext ctx, String expectedWarning) {
+ public ParseNode AnalyzesOk(String stmt, AnalysisContext ctx, String expectedWarning,
+ boolean assertNoWarnings) {
try (FrontendProfile.Scope scope = FrontendProfile.createNewWithScope()) {
- return feFixture_.analyzeStmt(stmt, ctx, expectedWarning);
+ return feFixture_.analyzeStmt(stmt, ctx, expectedWarning, assertNoWarnings);
}
}
+ public ParseNode AnalyzesOk(String stmt, AnalysisContext ctx, String expectedWarning) {
+ return AnalyzesOk(stmt, ctx, expectedWarning, false);
+ }
+
+ public ParseNode AnalyzesOkWithoutWarnings(String stmt, AnalysisContext ctx) {
+ return AnalyzesOk(stmt, ctx, null, true);
+ }
+
/**
* Analyzes the given statement without performing rewrites or authorization.
*/