Skip to content

HIVE-27133: Round off limit value greater than int_max to int_max #4110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

HIVE-27133: Round off limit value greater than int_max to int_max #4110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae81ac1
HIVE-27133: Round off limit value greater than int_max to int_max
Mar 7, 2023
3caa501
addressed comments - Removed extra line and fixed the typo in comments
Mar 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions common/src/java/org/apache/hive/common/util/HiveStringUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.io.PrintWriter;
import java.io.StringWriter;
import java.math.BigInteger;
import java.net.InetAddress;
import java.net.URI;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -1174,4 +1175,25 @@ private static boolean isComment(String line) {
return lineTrimmed.startsWith("#") || lineTrimmed.startsWith("--");
}

/**
* Returns integer value of a string. If the string value exceeds max int, returns Integer.MAX_VALUE
* else if the string value is less than min int, returns Integer.MIN_VALUE
*
* @param value value of the input string
* @return integer
*/
public static int convertStringToBoundedInt(String value) {
try {
BigInteger bigIntValue = new BigInteger(value);
if (bigIntValue.compareTo(BigInteger.valueOf(Integer.MAX_VALUE)) > 0) {
return Integer.MAX_VALUE;
Copy link
Contributor

@SourabhBadhya SourabhBadhya Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vamshikolanu
I agree with @jfsii. Converting a large number to Integer.MAX_VALUE is misleading the user.
Consider the following query -
INSERT INTO TABLE destinationTable SELECT * FROM sourceTable LIMIT <some_large_number>;
The insert will write records based on the output of the SELECT operator. In this case, since we have converted it to Integer.MAX_VALUE, the number of records written will be equal to Integer.MAX_VALUE which might not be what the user wants.

Perhaps adding a meaningful exception is better. In the long term, adding support for large integers for LIMIT clauses is even more better.

} else if ((bigIntValue.compareTo(BigInteger.valueOf(Integer.MIN_VALUE)) < 0)) {
return Integer.MIN_VALUE;
} else {
return bigIntValue.intValue();
}
} catch(NumberFormatException nfe){
throw new IllegalArgumentException("Please specify integer option. Provided option is " + value);
}
}
}
8 changes: 6 additions & 2 deletions ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,8 @@
import com.google.common.math.IntMath;
import com.google.common.math.LongMath;

import org.apache.hive.common.util.HiveStringUtils;

/**
* Implementation of the semantic analyzer. It generates the query plan.
* There are other specific semantic analyzers for some hive operations such as
Expand Down Expand Up @@ -1879,9 +1881,11 @@ boolean doPhase1(ASTNode ast, QB qb, Phase1Ctx ctx_1, PlannerContext plannerCtx)
queryProperties.setHasLimit(true);
if (ast.getChildCount() == 2) {
qbp.setDestLimit(ctx_1.dest,
Integer.valueOf(ast.getChild(0).getText()), Integer.valueOf(ast.getChild(1).getText()));
HiveStringUtils.convertStringToBoundedInt(ast.getChild(0).getText()),
HiveStringUtils.convertStringToBoundedInt(ast.getChild(1).getText()));
} else {
qbp.setDestLimit(ctx_1.dest, Integer.valueOf(0), Integer.valueOf(ast.getChild(0).getText()));
qbp.setDestLimit(ctx_1.dest, 0,
HiveStringUtils.convertStringToBoundedInt(ast.getChild(0).getText()));
}
break;

Expand Down
6 changes: 6 additions & 0 deletions ql/src/test/queries/clientpositive/limit_max_int.q
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
--! qt:dataset:src
select key from src limit 214748364700;
select key from src where key = '238' limit 214748364700;
select * from src where key = '238' limit 214748364700;
select src.key, count(src.value) from src group by src.key limit 214748364700;
select * from ( select key from src limit 3) sq1 limit 214748364700;
Copy link
Contributor

@jfsii jfsii Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to test underflow?

Copy link
Author

@vamshikolanu vamshikolanu Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

limit query fails for negative numbers at the parser level. Hence I haven't added it for a negative number. convertStringToBoundedInt() made it generic to handle both positive and negative cases.

Copy link
Contributor

@SourabhBadhya SourabhBadhya Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Please add a newline at the end of the qfile.

Loading