From 3d863b5ebff53ba5b8fcde922303643137f4b19b Mon Sep 17 00:00:00 2001 From: Hanifi Gunes Date: Thu, 5 Feb 2015 14:16:52 -0800 Subject: [PATCH] DRILL-2172: make web UI bound check before accessing a vector, returning null for overflowing indices; fix a non-string type rendering problem; --- .../exec/server/rest/QueryResources.java | 70 ++++------ .../drill/exec/server/rest/QueryWrapper.java | 126 ++++++++---------- .../src/main/resources/rest/query/result.ftl | 8 +- 3 files changed, 88 insertions(+), 116 deletions(-) diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryResources.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryResources.java index f8998ed72cc..9ef66768f3a 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryResources.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryResources.java @@ -17,7 +17,7 @@ */ package org.apache.drill.exec.server.rest; -import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; @@ -30,6 +30,7 @@ import javax.ws.rs.Produces; import javax.ws.rs.core.MediaType; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import org.apache.drill.common.config.DrillConfig; import org.apache.drill.exec.coord.ClusterCoordinator; @@ -55,7 +56,7 @@ public Viewable getQuery() { @Path("/query.json") @Consumes(MediaType.APPLICATION_JSON) @Produces(MediaType.APPLICATION_JSON) - public List> submitQueryJSON(QueryWrapper query) throws Exception { + public QueryWrapper.QueryResult submitQueryJSON(QueryWrapper query) throws Exception { final DrillConfig config = work.getContext().getConfig(); final ClusterCoordinator coordinator = work.getContext().getClusterCoordinator(); final BufferAllocator allocator = work.getContext().getAllocator(); @@ -67,58 +68,43 @@ public List> submitQueryJSON(QueryWrapper query) throws Exce @Consumes(MediaType.APPLICATION_FORM_URLENCODED) @Produces(MediaType.TEXT_HTML) public Viewable submitQuery(@FormParam("query") String query, @FormParam("queryType") String queryType) throws Exception { - List> result = submitQueryJSON(new QueryWrapper(query, queryType)); - - List columnNames; - if (result.isEmpty()) { - columnNames = Lists.newArrayList(); - } else { - columnNames = Lists.newArrayList(result.get(0).keySet()); - } - List> records = new ArrayList<>(); - - if(!isEmptyResult(result)) { - for (Map m : result) { - records.add(new ArrayList(m.values())); - } - } - - Table table = new Table(columnNames, records); - - return new Viewable("/rest/query/result.ftl", table); + final QueryWrapper.QueryResult result = submitQueryJSON(new QueryWrapper(query, queryType)); + return new Viewable("/rest/query/result.ftl", new TabularResult(result)); } - private boolean isEmptyResult(List> result) { - if (result.size() > 1) { - return false; - } else { - for(Object col : result.get(0).values()) { - if(col != null) { - return false; + public static class TabularResult { + private final List columns; + private final List> rows; + + public TabularResult(QueryWrapper.QueryResult result) { + final List> rows = Lists.newArrayList(); + for (Map rowMap:result.rows) { + final List row = Lists.newArrayList(); + for (String col:result.columns) { + row.add(rowMap.get(col)); } + rows.add(row); } - return true; + this.columns = ImmutableList.copyOf(result.columns); + this.rows = rows; } - } - public class Table { - private List columnNames; - private List> records; - - public Table(List columnNames, List> records) { - this.columnNames = columnNames; - this.records = records; + public TabularResult(List columns, List> rows) { + this.columns = columns; + this.rows = rows; } - public boolean isEmpty() { return getColumnNames().isEmpty(); } + public boolean isEmpty() { + return columns.isEmpty(); + } - public List getColumnNames() { - return columnNames; + public List getColumns() { + return columns; } - public List> getRecords() { - return records; + public List> getRows() { + return rows; } } } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java index 922533a1243..8996a695452 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java @@ -18,24 +18,23 @@ package org.apache.drill.exec.server.rest; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.LinkedList; +import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.atomic.AtomicInteger; import javax.xml.bind.annotation.XmlRootElement; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import org.apache.drill.common.config.DrillConfig; import org.apache.drill.exec.client.DrillClient; import org.apache.drill.exec.coord.ClusterCoordinator; import org.apache.drill.exec.exception.SchemaChangeException; import org.apache.drill.exec.memory.BufferAllocator; -import org.apache.drill.exec.physical.impl.flatten.FlattenRecordBatch; import org.apache.drill.exec.proto.UserBitShared; -import org.apache.drill.exec.record.MaterializedField; import org.apache.drill.exec.record.RecordBatchLoader; import org.apache.drill.exec.record.VectorWrapper; import org.apache.drill.exec.rpc.RpcException; @@ -43,10 +42,10 @@ import org.apache.drill.exec.rpc.user.QueryResultBatch; import org.apache.drill.exec.rpc.user.UserResultsListener; import org.apache.drill.exec.vector.ValueVector; -import org.apache.drill.exec.proto.UserBitShared.SerializedField; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import parquet.Preconditions; @XmlRootElement public class QueryWrapper { @@ -80,24 +79,36 @@ public UserBitShared.QueryType getType() { return type; } - public List> run(DrillConfig config, ClusterCoordinator coordinator, BufferAllocator allocator) + public QueryResult run(DrillConfig config, ClusterCoordinator coordinator, BufferAllocator allocator) throws Exception { try(DrillClient client = new DrillClient(config, coordinator, allocator)){ - Listener listener = new Listener(new RecordBatchLoader(allocator)); + Listener listener = new Listener(allocator); client.connect(); client.runQuery(getType(), query, listener); - List> result = listener.waitForCompletion(); - if (result.isEmpty()) { - Map dumbRecord = new HashMap<>(); - for (String columnName : listener.getColumnNames()) { - dumbRecord.put(columnName, null); + listener.waitForCompletion(); + if (listener.results.isEmpty()) { + listener.results.add(Maps.newHashMap()); + } + final Map first = listener.results.get(0); + for (String columnName : listener.columns) { + if (!first.containsKey(columnName)) { + first.put(columnName, null); } - result.add(dumbRecord); } - return result; + return new QueryResult(listener.columns, listener.results); + } + } + + public static class QueryResult { + public final Collection columns; + public final List> rows; + + public QueryResult(Collection columns, List> rows) { + this.columns = columns; + this.rows = rows; } } @@ -109,15 +120,13 @@ public String toString() { private static class Listener implements UserResultsListener { private volatile Exception exception; - private AtomicInteger count = new AtomicInteger(); - private CountDownLatch latch = new CountDownLatch(1); - private List> output = new LinkedList<>(); - private ArrayList columnNames; - private RecordBatchLoader loader; - private boolean schemaAdded = false; - - Listener(RecordBatchLoader loader) { - this.loader = loader; + private final CountDownLatch latch = new CountDownLatch(1); + private final BufferAllocator allocator; + public final List> results = Lists.newArrayList(); + public final Set columns = Sets.newLinkedHashSet(); + + Listener(BufferAllocator allocator) { + this.allocator = Preconditions.checkNotNull(allocator, "allocator cannot be null"); } @Override @@ -129,68 +138,45 @@ public void submissionFailed(RpcException ex) { @Override public void resultArrived(QueryResultBatch result, ConnectionThrottle throttle) { - int rows = result.getHeader().getRowCount(); - if (result.hasData()) { - count.addAndGet(rows); - try { + try { + final int rows = result.getHeader().getRowCount(); + if (result.hasData()) { + final RecordBatchLoader loader = new RecordBatchLoader(allocator); loader.load(result.getHeader().getDef(), result.getData()); - if (!schemaAdded || output.isEmpty()) { - columnNames = new ArrayList<>(); - for (int i = 0; i < loader.getSchema().getFieldCount(); ++i) { - columnNames.add(loader.getSchema().getColumn(i).getPath().getAsUnescapedPath()); - } - schemaAdded = true; + for (int i = 0; i < loader.getSchema().getFieldCount(); ++i) { + columns.add(loader.getSchema().getColumn(i).getPath().getAsUnescapedPath()); } - } catch (SchemaChangeException e) { - throw new RuntimeException(e); - } - for (int i = 0; i < rows; ++i) { - Map record = new HashMap<>(); - int j = 0; - for (VectorWrapper vw : loader) { - ValueVector.Accessor accessor = vw.getValueVector().getAccessor(); - Object object = accessor.getObject(i); - if (object != null && (! object.getClass().getName().startsWith("java.lang"))) { - object = object.toString(); - } - if (object != null) { - record.put(columnNames.get(j), object); - } else { - record.put(columnNames.get(j), null); + for (int i = 0; i < rows; ++i) { + final Map record = Maps.newHashMap(); + for (VectorWrapper vw : loader) { + final String field = vw.getValueVector().getMetadata().getNamePart().getName(); + final ValueVector.Accessor accessor = vw.getValueVector().getAccessor(); + final Object value = i < accessor.getValueCount() ? accessor.getObject(i) : null; + final String display = value == null ? null : value.toString(); + record.put(field, display); } - ++j; + results.add(record); } - output.add(record); } - } else if (!schemaAdded) { - columnNames = new ArrayList<>(); - schemaAdded = true; - for (SerializedField fmd : result.getHeader().getDef().getFieldList()) { - MaterializedField fieldDef = MaterializedField.create(fmd); - columnNames.add(fieldDef.getPath().getAsUnescapedPath()); + } catch (SchemaChangeException e) { + throw new RuntimeException(e); + } finally { + result.release(); + if (result.getHeader().getIsLastChunk()) { + latch.countDown(); } } - - result.release(); - if (result.getHeader().getIsLastChunk()) { - latch.countDown(); - } } @Override public void queryIdArrived(UserBitShared.QueryId queryId) { } - public List> waitForCompletion() throws Exception { + public void waitForCompletion() throws Exception { latch.await(); if (exception != null) { throw exception; } - return output; - } - - public List getColumnNames() { - return new ArrayList (columnNames); } } } diff --git a/exec/java-exec/src/main/resources/rest/query/result.ftl b/exec/java-exec/src/main/resources/rest/query/result.ftl index b69bd6d82f3..7fe52a4f0ee 100644 --- a/exec/java-exec/src/main/resources/rest/query/result.ftl +++ b/exec/java-exec/src/main/resources/rest/query/result.ftl @@ -33,16 +33,16 @@ - <#list model.getColumnNames() as value> - + <#list model.getColumns() as value> + - <#list model.getRecords() as record> + <#list model.getRows() as record> <#list record as value> - +
${value}${value?html}
<#if value??>${value}<#else>null${value!"null"?html}