Skip to content

Commit

Permalink
Merge pull request pentaho#797 from stanislau-strelchanka/MONDRIAN-2529
Browse files Browse the repository at this point in the history
[MONDRIAN-2529] ChooseByVolume=true can fail due to integer overflow
  • Loading branch information
Kurtis Walker authored Dec 14, 2016
2 parents d314f43 + f92410e commit 0d27426
Show file tree
Hide file tree
Showing 12 changed files with 115 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2773,7 +2773,7 @@ public void testAggStarWithIgnoredColumnsRequiresRollup() {
AggStar aggStarSpy = spy(
getAggStar(star, "agg_c_10_sales_fact_1997"));
// make sure the test AggStar will be prioritized first
when(aggStarSpy.getSize()).thenReturn(0);
when(aggStarSpy.getSize()).thenReturn(0l);
context.getConnection().getSchemaReader()
.getSchema().getStar("sales_fact_1997").addAggStar(aggStarSpy);
boolean[] rollup = { false };
Expand Down Expand Up @@ -2848,7 +2848,7 @@ public void testAggStarWithUnusedColumnsRequiresRollup() {
AggStar aggStarSpy = spy(
getAggStar(star, "agg_c_special_sales_fact_1997"));
// make sure the test AggStar will be prioritized first
when(aggStarSpy.getSize()).thenReturn(0);
when(aggStarSpy.getSize()).thenReturn(0l);
context.getConnection().getSchemaReader()
.getSchema().getStar("sales_fact_1997").addAggStar(aggStarSpy);

Expand Down Expand Up @@ -2945,7 +2945,7 @@ public void testAggStarWithIgnoredColumnsAndCountDistinct() {
AggStar aggStarSpy = spy(
getAggStar(star, "agg_g_ms_pcat_sales_fact_1997"));
// make sure the test AggStar will be prioritized first
when(aggStarSpy.getSize()).thenReturn(0);
when(aggStarSpy.getSize()).thenReturn(0l);
context.getConnection().getSchemaReader()
.getSchema().getStar("sales_fact_1997").addAggStar(aggStarSpy);
boolean[] rollup = { false };
Expand Down
60 changes: 60 additions & 0 deletions mondrian/src/it/java/mondrian/rolap/agg/AggStarTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
// This software is subject to the terms of the Eclipse Public License v1.0
// Agreement, available at the following URL:
// http://www.eclipse.org/legal/epl-v10.html.
// You must accept the terms of that agreement to use this software.
//
// Copyright (C) 2003-2005 Julian Hyde
// Copyright (C) 2005-2016 Pentaho
// All Rights Reserved.
*/
package mondrian.rolap.agg;

import junit.framework.Assert;
import mondrian.recorder.MessageRecorder;
import mondrian.rolap.RolapStar;
import mondrian.rolap.aggmatcher.AggStar;
import mondrian.rolap.aggmatcher.JdbcSchema;
import mondrian.test.PropertyRestoringTestCase;
import org.mockito.Mockito;

public class AggStarTest extends PropertyRestoringTestCase {

private AggStar aggStar;
private RolapStar star;
private JdbcSchema.Table table;
private MessageRecorder messageRecorder;
private static final Long BIG_NUMBER = Integer.MAX_VALUE + 1L;

@Override
protected void setUp() throws Exception {
super.setUp();
star = Mockito.mock(RolapStar.class);
table = Mockito.mock(JdbcSchema.Table.class);
messageRecorder = Mockito.mock(MessageRecorder.class);

Mockito.when(table.getColumnUsages(Mockito.any())).thenCallRealMethod();
Mockito.when(table.getName()).thenReturn("TestAggTable");
Mockito.when(table.getTotalColumnSize()).thenReturn(1);
Mockito.when(table.getNumberOfRows()).thenReturn(BIG_NUMBER);

aggStar = AggStar.makeAggStar(star, table, messageRecorder, 0L);
aggStar = Mockito.spy(aggStar);

propSaver.set(propSaver.properties.ChooseAggregateByVolume, false);
}

@Override
protected void tearDown() throws Exception {
super.tearDown();
}

public void testSizeIntegerOverflow() {
Assert.assertEquals(BIG_NUMBER.longValue(), aggStar.getSize());
}

public void testVolumeIntegerOverflow() {
propSaver.set(propSaver.properties.ChooseAggregateByVolume, true);
Assert.assertEquals(BIG_NUMBER.longValue(), aggStar.getSize());
}
}
4 changes: 2 additions & 2 deletions mondrian/src/it/java/mondrian/test/BasicQueryTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -7913,7 +7913,7 @@ public void testStatistics() {
statisticsProviders.get(1) instanceof SqlStatisticsProvider);

for (StatisticsProvider statisticsProvider : statisticsProviders) {
int rowCount =
long rowCount =
statisticsProvider.getTableCardinality(
dialect,
testContext.getConnection().getDataSource(),
Expand All @@ -7932,7 +7932,7 @@ public void testStatistics() {
rowCount > 10000 && rowCount < 15000);
}

int valueCount =
long valueCount =
statisticsProvider.getColumnCardinality(
dialect,
testContext.getConnection().getDataSource(),
Expand Down
12 changes: 6 additions & 6 deletions mondrian/src/main/java/mondrian/rolap/RolapStar.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// You must accept the terms of that agreement to use this software.
//
// Copyright (C) 2001-2005 Julian Hyde
// Copyright (C) 2005-2013 Pentaho and others
// Copyright (C) 2005-2016 Pentaho and others
// All Rights Reserved.
//
// jhyde, 12 August, 2001
Expand All @@ -30,7 +30,7 @@
import java.sql.Connection;
import java.sql.*;
import java.util.*;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;

import javax.sql.DataSource;

Expand Down Expand Up @@ -433,7 +433,7 @@ public void prepareToLoadAggregates() {
*/
public void addAggStar(AggStar aggStar) {
// Add it before the first AggStar which is larger, if there is one.
int size = aggStar.getSize();
long size = aggStar.getSize();
ListIterator<AggStar> lit = aggStars.listIterator();
while (lit.hasNext()) {
AggStar as = lit.next();
Expand Down Expand Up @@ -878,8 +878,8 @@ public int compare(
* The estimated cardinality of the column.
* {@link Integer#MIN_VALUE} means unknown.
*/
private AtomicInteger approxCardinality = new AtomicInteger(
Integer.MIN_VALUE);
private AtomicLong approxCardinality = new AtomicLong(
Long.MIN_VALUE);

private Column(
String name,
Expand Down Expand Up @@ -1018,7 +1018,7 @@ public String generateExprString(SqlQuery query) {
*
* @return the column cardinality.
*/
public int getCardinality() {
public long getCardinality() {
if (approxCardinality.get() < 0) {
approxCardinality.set(
table.star.getStatisticsCache().getColumnCardinality(
Expand Down
30 changes: 15 additions & 15 deletions mondrian/src/main/java/mondrian/rolap/RolapStatisticsCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* http://www.eclipse.org/legal/epl-v10.html.
* You must accept the terms of that agreement to use this software.
*
* Copyright (c) 2002-2013 Pentaho Corporation.. All rights reserved.
* Copyright (c) 2002-2016 Pentaho Corporation.. All rights reserved.
*/

package mondrian.rolap;
Expand All @@ -26,19 +26,19 @@
*/
public class RolapStatisticsCache {
private final RolapStar star;
private final Map<List, Integer> columnMap = new HashMap<List, Integer>();
private final Map<List, Integer> tableMap = new HashMap<List, Integer>();
private final Map<String, Integer> queryMap =
new HashMap<String, Integer>();
private final Map<List, Long> columnMap = new HashMap<List, Long>();
private final Map<List, Long> tableMap = new HashMap<List, Long>();
private final Map<String, Long> queryMap =
new HashMap<String, Long>();

public RolapStatisticsCache(RolapStar star) {
this.star = star;
}

public int getRelationCardinality(
public long getRelationCardinality(
MondrianDef.Relation relation,
String alias,
int approxRowCount)
long approxRowCount)
{
if (approxRowCount >= 0) {
return approxRowCount;
Expand All @@ -55,13 +55,13 @@ public int getRelationCardinality(
}
}

private int getTableCardinality(
private long getTableCardinality(
String catalog,
String schema,
String table)
{
final List<String> key = Arrays.asList(catalog, schema, table);
int rowCount = -1;
long rowCount = -1;
if (tableMap.containsKey(key)) {
rowCount = tableMap.get(key);
} else {
Expand Down Expand Up @@ -93,8 +93,8 @@ private int getTableCardinality(
return rowCount;
}

private int getQueryCardinality(String sql) {
int rowCount = -1;
private long getQueryCardinality(String sql) {
long rowCount = -1;
if (queryMap.containsKey(sql)) {
rowCount = queryMap.get(sql);
} else {
Expand All @@ -121,10 +121,10 @@ private int getQueryCardinality(String sql) {
return rowCount;
}

public int getColumnCardinality(
public long getColumnCardinality(
MondrianDef.Relation relation,
MondrianDef.Expression expression,
int approxCardinality)
long approxCardinality)
{
if (approxCardinality >= 0) {
return approxCardinality;
Expand All @@ -148,14 +148,14 @@ public int getColumnCardinality(
}
}

private int getColumnCardinality(
private long getColumnCardinality(
String catalog,
String schema,
String table,
String column)
{
final List<String> key = Arrays.asList(catalog, schema, table, column);
int rowCount = -1;
long rowCount = -1;
if (columnMap.containsKey(key)) {
rowCount = columnMap.get(key);
} else {
Expand Down
18 changes: 9 additions & 9 deletions mondrian/src/main/java/mondrian/rolap/aggmatcher/AggStar.java
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static AggStar makeAggStar(
final RolapStar star,
final JdbcSchema.Table dbTable,
final MessageRecorder msgRecorder,
final int approxRowCount)
final long approxRowCount)
{
AggStar aggStar = new AggStar(star, dbTable, approxRowCount);
AggStar.FactTable aggStarFactTable = aggStar.getFactTable();
Expand Down Expand Up @@ -235,12 +235,12 @@ private static void collectLevels(
/**
* An approximate number of rows present in this aggregate table.
*/
private final int approxRowCount;
private final long approxRowCount;

AggStar(
final RolapStar star,
final JdbcSchema.Table aggTable,
final int approxRowCount)
final long approxRowCount)
{
this.star = star;
this.approxRowCount = approxRowCount;
Expand Down Expand Up @@ -282,7 +282,7 @@ public Table findTable(String name) {
* If the property {@link MondrianProperties#ChooseAggregateByVolume}
* is true, then volume is returned, otherwise row count.
*/
public int getSize() {
public long getSize() {
return MondrianProperties.instance().ChooseAggregateByVolume.get()
? getFactTable().getVolume()
: getFactTable().getNumberOfRows();
Expand Down Expand Up @@ -1157,7 +1157,7 @@ public String generateExprString(SqlQuery query) {
private Set<Column> measureFactCountColumns = new HashSet<>();
private final List<Measure> measures;
private final int totalColumnSize;
private int numberOfRows;
private long numberOfRows;

FactTable(final JdbcSchema.Table aggTable) {
this(
Expand All @@ -1171,7 +1171,7 @@ public String generateExprString(SqlQuery query) {
final String name,
final MondrianDef.Relation relation,
final int totalColumnSize,
final int numberOfRows)
final long numberOfRows)
{
super(name, relation);
this.totalColumnSize = totalColumnSize;
Expand All @@ -1198,21 +1198,21 @@ public Table.JoinCondition getJoinCondition() {
/**
* Get the volume of the table (now of rows * size of a row).
*/
public int getVolume() {
public long getVolume() {
return getTotalColumnSize() * getNumberOfRows();
}

/**
* Get the total size of all columns in a row.
*/
public int getTotalColumnSize() {
public long getTotalColumnSize() {
return totalColumnSize;
}

/**
* Get the number of rows in this aggregate table.
*/
public int getNumberOfRows() {
public long getNumberOfRows() {
if (numberOfRows < 0) {
numberOfRows =
star.getStatisticsCache().getRelationCardinality(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ private static void sweepDB() {
/**
* Enumeration of ways that an aggregate table can use a column.
*/
enum UsageType {
public enum UsageType {
UNKNOWN,
FOREIGN_KEY,
MEASURE,
Expand Down Expand Up @@ -907,7 +907,7 @@ public int getTotalColumnSize() {
/**
* Returns the number of rows in the table.
*/
public int getNumberOfRows() {
public long getNumberOfRows() {
return -1;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// http://www.eclipse.org/legal/epl-v10.html.
// You must accept the terms of that agreement to use this software.
//
// Copyright (c) 2002-2015 Pentaho Corporation.
// Copyright (c) 2002-2016 Pentaho Corporation.
// All Rights Reserved.
*/
package mondrian.rolap.cache;
Expand Down Expand Up @@ -868,7 +868,7 @@ private void findRollupCandidatesAmong(
for (Pair<SegmentHeader, List<SegmentColumn>> pair : matchingHeaders) {
for (SegmentColumn column : pair.right) {
if (!columnNameList.contains(column.columnExpression)) {
final int valueCount = column.getValueCount();
final long valueCount = column.getValueCount();
if (valueCount <= 0) {
// Impossible to safely roll up. If we don't know the
// number of values, we don't know that we have all of
Expand Down
8 changes: 4 additions & 4 deletions mondrian/src/main/java/mondrian/spi/SegmentColumn.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* http://www.eclipse.org/legal/epl-v10.html.
* You must accept the terms of that agreement to use this software.
*
* Copyright (c) 2002-2013 Pentaho Corporation.. All rights reserved.
* Copyright (c) 2002-2016 Pentaho Corporation.. All rights reserved.
*/

package mondrian.spi;
Expand All @@ -28,7 +28,7 @@
public class SegmentColumn implements Serializable {
private static final long serialVersionUID = -5227838916517784720L;
public final String columnExpression;
public final int valueCount;
public final long valueCount;
public final SortedSet<Comparable> values;
private final int hashCode;

Expand All @@ -52,7 +52,7 @@ public class SegmentColumn implements Serializable {
*/
public SegmentColumn(
String columnExpression,
int valueCount,
long valueCount,
SortedSet<Comparable> valueList)
{
this.columnExpression = columnExpression;
Expand Down Expand Up @@ -139,7 +139,7 @@ public int hashCode() {
* @return Number of distinct values, including null, that occur for this
* column
*/
public int getValueCount() {
public long getValueCount() {
return valueCount;
}
}
Expand Down
Loading

0 comments on commit 0d27426

Please sign in to comment.