ayushtkn commented on code in PR #7222:
URL: https://github.com/apache/ozone/pull/7222#discussion_r1769185866
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBScanner.java:
##########
@@ -328,6 +355,152 @@ private void processRecords(ManagedRocksIterator iterator,
}
}
+ private void getFilterSplit(List<String> fields, Map<String, Filter>
fieldMap, Filter leafValue) throws IOException {
+ int len = fields.size();
+ if (len == 1) {
+ Filter currentValue = fieldMap.get(fields.get(0));
+ if (currentValue != null) {
+ err().println("Cannot pass multiple values for the same field and " +
+ "cannot have filter for both parent and child");
+ throw new IOException("Invalid filter passed");
+ }
+ fieldMap.put(fields.get(0), leafValue);
+ } else {
+ Filter fieldMapGet = fieldMap.computeIfAbsent(fields.get(0), k -> new
Filter());
+ if (fieldMapGet.getValue() != null) {
+ err().println("Cannot pass multiple values for the same field and " +
+ "cannot have filter for both parent and child");
+ throw new IOException("Invalid filter passed");
+ }
+ Map<String, Filter> nextLevel = fieldMapGet.getNextLevel();
+ if (nextLevel == null) {
+ fieldMapGet.setNextLevel(new HashMap<>());
+ }
+ getFilterSplit(fields.subList(1, len), fieldMapGet.getNextLevel(),
leafValue);
+ }
+ }
+
+ private boolean checkFilteredObject(Object obj, Class<?> clazz, Map<String,
Filter> fieldsSplitMap)
+ throws IOException {
+ for (Map.Entry<String, Filter> field : fieldsSplitMap.entrySet()) {
+ try {
+ Field valueClassField = getRequiredFieldFromAllFields(clazz,
field.getKey());
+ Object valueObject = valueClassField.get(obj);
+ Filter fieldValue = field.getValue();
+
+ if (valueObject == null) {
+ // there is no such field in the record. This filter will be ignored
for the current record.
+ continue;
+ }
+ if (fieldValue == null) {
+ err().println("Malformed filter. Check input");
+ throw new IOException("Invalid filter passed");
+ } else if (fieldValue.getNextLevel() == null) {
+ // reached the end of fields hierarchy, check if they match the
filter
+ // Currently, only equals operation is supported
+ try {
+ switch (fieldValue.getOperator()) {
+ case EQUALS:
+ if (!String.valueOf(valueObject).equals(fieldValue.getValue())) {
+ return false;
+ }
+ break;
+ case GREATER:
+ if (Double.parseDouble(String.valueOf(valueObject))
+ < Double.parseDouble(String.valueOf(fieldValue.getValue())))
{
+ return false;
+ }
+ break;
+ case LESSER:
+ if (Double.parseDouble(String.valueOf(valueObject))
+ > Double.parseDouble(String.valueOf(fieldValue.getValue())))
{
+ return false;
+ }
+ break;
+ default:
+ err().println("Only EQUALS/LESSER/GREATER operator is supported
currently.");
+ throw new IOException("Invalid filter passed");
+ }
+ } catch (NumberFormatException ex) {
+ err().println("LESSER or GREATER operation can be performed only
on numeric values.");
+ throw new IOException("Invalid filter passed");
+ }
+ } else {
+ Map<String, Filter> subfields = fieldValue.getNextLevel();
+ if (Collection.class.isAssignableFrom(valueObject.getClass())) {
+ if (!checkFilteredObjectCollection((Collection) valueObject,
subfields)) {
+ return false;
+ }
+ } else if (Map.class.isAssignableFrom(valueObject.getClass())) {
+ Map<?, ?> valueObjectMap = (Map<?, ?>) valueObject;
+ boolean flag = false;
+ for (Map.Entry<?, ?> ob : valueObjectMap.entrySet()) {
+ boolean subflag;
+ if (Collection.class.isAssignableFrom(ob.getValue().getClass()))
{
+ subflag =
checkFilteredObjectCollection((Collection)ob.getValue(), subfields);
+ } else {
+ subflag = checkFilteredObject(ob.getValue(),
ob.getValue().getClass(), subfields);
+ }
+ if (subflag) {
+ // atleast one item in the map/list of the record has matched
the filter,
+ // so record passes the filter.
+ flag = true;
+ break;
+ }
+ }
+ if (!flag) {
+ // none of the items in the map/list passed the filter => record
doesn't pass the filter
+ return false;
+ }
+ } else {
+ if (!checkFilteredObject(valueObject, valueClassField.getType(),
subfields)) {
+ return false;
+ }
+ }
+ }
+ } catch (NoSuchFieldException ex) {
+ err().println("ERROR: no such field: " + field);
+ exception = true;
+ return false;
+ } catch (IllegalAccessException e) {
+ err().println("ERROR: Cannot get field from object: " + field);
Review Comment:
this ain't very right, this communicates as if the object: name is given by
field
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBScanner.java:
##########
@@ -132,7 +132,7 @@ public class DBScanner implements Callable<Void>,
SubcommandWithParent {
@CommandLine.Option(names = {"--filter"},
description = "Comma-separated list of \"<field>:<operator>:<value>\"
where " +
"<field> is any valid field of the record, " +
- "<operator> is (EQUALS,MAX or MIN) and " +
+ "<operator> is (EQUALS,LESSER or GREATER) and " +
Review Comment:
Why are removing `MAX` & `MIN` operators? that too in the scope of
introducing 2 new operators?
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBScanner.java:
##########
@@ -289,33 +289,60 @@ private void processRecords(ManagedRocksIterator iterator,
long count = 0;
List<Future<Void>> futures = new ArrayList<>();
boolean reachedEnd = false;
+
+ Map<String, Filter> fieldsFilterSplitMap = new HashMap<>();
+ if (filter != null) {
+ for (String field : filter.split(",")) {
+ String[] fieldValue = field.split(":");
+ if (fieldValue.length != 3) {
+ err().println("Error: Invalid format for filter \"" + field
+ + "\". Usage: <field>:<operator>:<value>. Ignoring filter
passed");
+ } else {
+ Filter filterValue = new Filter(fieldValue[1], fieldValue[2]);
+ if (filterValue.getOperator() == null) {
+ err().println("Error: Invalid format for filter \"" + filterValue
Review Comment:
This isn't invalid `format` for filter, but invalid `operator` for the filter
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBScanner.java:
##########
@@ -289,33 +289,60 @@ private void processRecords(ManagedRocksIterator iterator,
long count = 0;
List<Future<Void>> futures = new ArrayList<>();
boolean reachedEnd = false;
+
+ Map<String, Filter> fieldsFilterSplitMap = new HashMap<>();
+ if (filter != null) {
+ for (String field : filter.split(",")) {
Review Comment:
what is being split by ``,`` here, your filter would be like
``dataSize:lesser:500`` and you can't specify multiple filters by this code if
you do so something like ``dataSize:lesser:500, dataSize:equal:500`` this
crashes.
Can you help what is this doing? & maybe extend a test case which uses this,
if not already
I removed this for loop itself & no test fails in ``TestLDBCLI``, I guess
the intention might be to support multiple filters maybe, if so you need to fix
it
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBScanner.java:
##########
@@ -328,6 +355,152 @@ private void processRecords(ManagedRocksIterator iterator,
}
}
+ private void getFilterSplit(List<String> fields, Map<String, Filter>
fieldMap, Filter leafValue) throws IOException {
+ int len = fields.size();
+ if (len == 1) {
+ Filter currentValue = fieldMap.get(fields.get(0));
+ if (currentValue != null) {
+ err().println("Cannot pass multiple values for the same field and " +
+ "cannot have filter for both parent and child");
+ throw new IOException("Invalid filter passed");
+ }
+ fieldMap.put(fields.get(0), leafValue);
+ } else {
+ Filter fieldMapGet = fieldMap.computeIfAbsent(fields.get(0), k -> new
Filter());
+ if (fieldMapGet.getValue() != null) {
+ err().println("Cannot pass multiple values for the same field and " +
+ "cannot have filter for both parent and child");
+ throw new IOException("Invalid filter passed");
+ }
+ Map<String, Filter> nextLevel = fieldMapGet.getNextLevel();
+ if (nextLevel == null) {
+ fieldMapGet.setNextLevel(new HashMap<>());
+ }
+ getFilterSplit(fields.subList(1, len), fieldMapGet.getNextLevel(),
leafValue);
+ }
+ }
+
+ private boolean checkFilteredObject(Object obj, Class<?> clazz, Map<String,
Filter> fieldsSplitMap)
+ throws IOException {
Review Comment:
this doesn't throw IOE
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBScanner.java:
##########
@@ -328,6 +355,152 @@ private void processRecords(ManagedRocksIterator iterator,
}
}
+ private void getFilterSplit(List<String> fields, Map<String, Filter>
fieldMap, Filter leafValue) throws IOException {
+ int len = fields.size();
+ if (len == 1) {
+ Filter currentValue = fieldMap.get(fields.get(0));
+ if (currentValue != null) {
+ err().println("Cannot pass multiple values for the same field and " +
+ "cannot have filter for both parent and child");
+ throw new IOException("Invalid filter passed");
+ }
+ fieldMap.put(fields.get(0), leafValue);
+ } else {
+ Filter fieldMapGet = fieldMap.computeIfAbsent(fields.get(0), k -> new
Filter());
+ if (fieldMapGet.getValue() != null) {
+ err().println("Cannot pass multiple values for the same field and " +
+ "cannot have filter for both parent and child");
+ throw new IOException("Invalid filter passed");
+ }
+ Map<String, Filter> nextLevel = fieldMapGet.getNextLevel();
+ if (nextLevel == null) {
+ fieldMapGet.setNextLevel(new HashMap<>());
+ }
+ getFilterSplit(fields.subList(1, len), fieldMapGet.getNextLevel(),
leafValue);
+ }
+ }
+
+ private boolean checkFilteredObject(Object obj, Class<?> clazz, Map<String,
Filter> fieldsSplitMap)
+ throws IOException {
+ for (Map.Entry<String, Filter> field : fieldsSplitMap.entrySet()) {
+ try {
+ Field valueClassField = getRequiredFieldFromAllFields(clazz,
field.getKey());
+ Object valueObject = valueClassField.get(obj);
+ Filter fieldValue = field.getValue();
+
+ if (valueObject == null) {
+ // there is no such field in the record. This filter will be ignored
for the current record.
+ continue;
+ }
+ if (fieldValue == null) {
+ err().println("Malformed filter. Check input");
+ throw new IOException("Invalid filter passed");
+ } else if (fieldValue.getNextLevel() == null) {
+ // reached the end of fields hierarchy, check if they match the
filter
+ // Currently, only equals operation is supported
+ try {
+ switch (fieldValue.getOperator()) {
+ case EQUALS:
+ if (!String.valueOf(valueObject).equals(fieldValue.getValue())) {
+ return false;
+ }
+ break;
+ case GREATER:
+ if (Double.parseDouble(String.valueOf(valueObject))
+ < Double.parseDouble(String.valueOf(fieldValue.getValue())))
{
+ return false;
+ }
+ break;
+ case LESSER:
+ if (Double.parseDouble(String.valueOf(valueObject))
+ > Double.parseDouble(String.valueOf(fieldValue.getValue())))
{
+ return false;
+ }
+ break;
+ default:
+ err().println("Only EQUALS/LESSER/GREATER operator is supported
currently.");
+ throw new IOException("Invalid filter passed");
+ }
+ } catch (NumberFormatException ex) {
+ err().println("LESSER or GREATER operation can be performed only
on numeric values.");
+ throw new IOException("Invalid filter passed");
+ }
+ } else {
+ Map<String, Filter> subfields = fieldValue.getNextLevel();
+ if (Collection.class.isAssignableFrom(valueObject.getClass())) {
+ if (!checkFilteredObjectCollection((Collection) valueObject,
subfields)) {
+ return false;
+ }
+ } else if (Map.class.isAssignableFrom(valueObject.getClass())) {
+ Map<?, ?> valueObjectMap = (Map<?, ?>) valueObject;
+ boolean flag = false;
+ for (Map.Entry<?, ?> ob : valueObjectMap.entrySet()) {
+ boolean subflag;
+ if (Collection.class.isAssignableFrom(ob.getValue().getClass()))
{
+ subflag =
checkFilteredObjectCollection((Collection)ob.getValue(), subfields);
+ } else {
+ subflag = checkFilteredObject(ob.getValue(),
ob.getValue().getClass(), subfields);
+ }
+ if (subflag) {
+ // atleast one item in the map/list of the record has matched
the filter,
+ // so record passes the filter.
+ flag = true;
+ break;
+ }
+ }
+ if (!flag) {
+ // none of the items in the map/list passed the filter => record
doesn't pass the filter
+ return false;
+ }
+ } else {
+ if (!checkFilteredObject(valueObject, valueClassField.getType(),
subfields)) {
+ return false;
+ }
+ }
+ }
+ } catch (NoSuchFieldException ex) {
+ err().println("ERROR: no such field: " + field);
+ exception = true;
+ return false;
+ } catch (IllegalAccessException e) {
+ err().println("ERROR: Cannot get field from object: " + field);
+ exception = true;
+ return false;
+ } catch (Exception ex) {
+ err().println("ERROR: field: " + field + ", ex: " + ex);
+ exception = true;
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private boolean checkFilteredObjectCollection(Collection<?> valueObject,
Map<String, Filter> fields)
+ throws NoSuchFieldException, IllegalAccessException, IOException {
+ for (Object ob : valueObject) {
+ if (checkFilteredObject(ob, ob.getClass(), fields)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ static Field getRequiredFieldFromAllFields(Class clazz, String fieldName)
throws NoSuchFieldException {
+ List<Field> classFieldList = ValueSchema.getAllFields(clazz);
+ Field classField = null;
+ for (Field f : classFieldList) {
+ if (f.getName().equals(fieldName)) {
+ classField = f;
+ break;
+ }
+ }
+ if (classField == null) {
+ throw new NoSuchFieldException();
Review Comment:
I think we should put some message as well, which can tell which field not
found in which class or so
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java:
##########
@@ -182,6 +182,30 @@ private static Stream<Arguments> scanTestCases() {
Named.of("Filter invalid key", Arrays.asList("--filter",
"keyName:equals:key9")),
Named.of("Expect key1-key3", null)
),
+ Arguments.of(
+ Named.of(KEY_TABLE, Pair.of(KEY_TABLE, false)),
+ Named.of("Default", Pair.of(0, "")),
+ Named.of("Filter dataSize<2000", Arrays.asList("--filter",
"dataSize:lesser:2000")),
+ Named.of("Expect key1-key5", Pair.of("key1", "key6"))
+ ),
+ Arguments.of(
+ Named.of(KEY_TABLE, Pair.of(KEY_TABLE, false)),
+ Named.of("Default", Pair.of(0, "")),
+ Named.of("Filter dataSize<500", Arrays.asList("--filter",
"dataSize:lesser:500")),
+ Named.of("Expect empty result", null)
+ ),
+ Arguments.of(
+ Named.of(KEY_TABLE, Pair.of(KEY_TABLE, false)),
+ Named.of("Default", Pair.of(0, "")),
+ Named.of("Filter dataSize>500", Arrays.asList("--filter",
"dataSize:greater:500")),
+ Named.of("Expect key1-key5", Pair.of("key1", "key6"))
+ ),
+ Arguments.of(
+ Named.of(KEY_TABLE, Pair.of(KEY_TABLE, false)),
+ Named.of("Default", Pair.of(0, "")),
+ Named.of("Filter dataSize>2000", Arrays.asList("--filter",
"dataSize:greater:2000")),
+ Named.of("Expect empty result", null)
+ ),
Review Comment:
should add some -ve test cases somewhere as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
