You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by kaspersorensen <gi...@git.apache.org> on 2017/10/20 23:29:26 UTC
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
GitHub user kaspersorensen opened a pull request:
https://github.com/apache/metamodel/pull/165
METAMODEL-1165: Add alias default table
Another approach to METAMODEL-1165. This time with the use of alias tables. Following the feedback from @LosD and @tomatophantastico from my previous PR on the same issue.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kaspersorensen/metamodel METAMODEL-1165-add-alias-default-table
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/165.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #165
----
commit 0dbecf1fca722fe0bbbb86cfdd3dc2a800ccaacc
Author: Kasper Sørensen <i....@gmail.com>
Date: 2017-10-18T21:43:25Z
METAMODEL-1165: New approach to solution - adding alias table
commit 4668750369062ff5b82e7bc51221a36ad25e9720
Author: Kasper Sørensen <i....@gmail.com>
Date: 2017-10-18T22:09:07Z
METAMODEL-1165: Improved wrapping of schema and table objects
commit dcf202fefb0d1ab07d0604b2d29d3eb75a318bae
Author: Kasper Sørensen <i....@gmail.com>
Date: 2017-10-20T10:55:55Z
METAMODEL-1165: Applied fix to other data context implementations
----
---
[GitHub] metamodel issue #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/165
I also thought the change would be a bit easier. But we've never had anything like it before so nothing was really prepared for it.
I think actually this is a rather significant feature. The inconvenience of not being able to easily formulate a simple "SELECT * FROM table" on a CSV file is really annoying. - Especially if we look to the Membrane webapp where I could totally see a use case in using this type of query. After all, when you as a HTTP client only have SQL-like strings to query with, convenience and syntax sugar becomes very important. And it makes the Postman test cases a lot more portable too ;-)
---
[GitHub] metamodel issue #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/165
Thank you for the review. And sorry for the needless diffs. But now that things are straightened out, I'll merge ;-)
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148174162
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -63,24 +64,29 @@
import org.slf4j.LoggerFactory;
/**
- * Abstract DataContext for data sources that do not support SQL queries
- * natively.
+ * Abstract DataContext for data sources that do not support SQL queries natively.
*
- * Instead this superclass only requires that a subclass can materialize a
- * single table at a time. Then the query will be executed by post processing
- * the datasets client-side.
+ * Instead this superclass only requires that a subclass can materialize a single table at a time. Then the query will
+ * be executed by post processing the datasets client-side.
*/
public abstract class QueryPostprocessDataContext extends AbstractDataContext implements HasReadTypeConverters {
private static final Logger logger = LoggerFactory.getLogger(QueryPostprocessDataContext.class);
+ public static final String SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS = "metamodel.alias.default.table";
public static final String INFORMATION_SCHEMA_NAME = "information_schema";
- private final Map<Column, TypeConverter<?, ?>> _converters;
+ private final Map<Column, TypeConverter<?, ?>> converters;
--- End diff --
I did it for consistency sake actually. Since I introduced a new field (without the underscore, which I know you don't like) I felt like renaming the other fields in the class to also not have underscores would be appropriate.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148356156
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -63,24 +64,29 @@
import org.slf4j.LoggerFactory;
/**
- * Abstract DataContext for data sources that do not support SQL queries
- * natively.
+ * Abstract DataContext for data sources that do not support SQL queries natively.
*
- * Instead this superclass only requires that a subclass can materialize a
- * single table at a time. Then the query will be executed by post processing
- * the datasets client-side.
+ * Instead this superclass only requires that a subclass can materialize a single table at a time. Then the query will
+ * be executed by post processing the datasets client-side.
*/
public abstract class QueryPostprocessDataContext extends AbstractDataContext implements HasReadTypeConverters {
private static final Logger logger = LoggerFactory.getLogger(QueryPostprocessDataContext.class);
+ public static final String SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS = "metamodel.alias.default.table";
public static final String INFORMATION_SCHEMA_NAME = "information_schema";
- private final Map<Column, TypeConverter<?, ?>> _converters;
+ private final Map<Column, TypeConverter<?, ?>> converters;
+ private final boolean defaultTableEnabled;
public QueryPostprocessDataContext() {
+ this(true);
--- End diff --
Fair enough.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148126979
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -235,38 +240,28 @@ private boolean isSimpleSelect(SelectClause clause) {
}
/**
- * Executes a simple count query, if possible. This method is provided to
- * allow subclasses to optimize count queries since they are quite common
- * and often a datastore can retrieve the count using some specialized means
- * which is much more performant than counting all records manually.
+ * Executes a simple count query, if possible. This method is provided to allow subclasses to optimize count queries
+ * since they are quite common and often a datastore can retrieve the count using some specialized means which is
+ * much more performant than counting all records manually.
--- End diff --
Unrelated change.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148126961
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -216,8 +222,7 @@ public DataSet executeQuery(final Query query) {
}
/**
- * Determines if all the select items are 'simple' meaning that they just
- * represent scans of values in columns.
+ * Determines if all the select items are 'simple' meaning that they just represent scans of values in columns.
--- End diff --
Unrelated change.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148127296
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -63,24 +64,29 @@
import org.slf4j.LoggerFactory;
/**
- * Abstract DataContext for data sources that do not support SQL queries
- * natively.
+ * Abstract DataContext for data sources that do not support SQL queries natively.
*
- * Instead this superclass only requires that a subclass can materialize a
- * single table at a time. Then the query will be executed by post processing
- * the datasets client-side.
+ * Instead this superclass only requires that a subclass can materialize a single table at a time. Then the query will
+ * be executed by post processing the datasets client-side.
*/
public abstract class QueryPostprocessDataContext extends AbstractDataContext implements HasReadTypeConverters {
private static final Logger logger = LoggerFactory.getLogger(QueryPostprocessDataContext.class);
+ public static final String SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS = "metamodel.alias.default.table";
public static final String INFORMATION_SCHEMA_NAME = "information_schema";
- private final Map<Column, TypeConverter<?, ?>> _converters;
+ private final Map<Column, TypeConverter<?, ?>> converters;
--- End diff --
Unrelated change.
---
[GitHub] metamodel issue #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/165
In the one I looked at (QueryPostProcessessDataContext.java), the actual changes were around 10-15 lines (a little more with the since-introduced JavaDoc), out of 175 lines changed total, in a 700 lines class. A quick glance seems to repeat those numbers for many other classes.
Reformatting is fine.... For the method you're changing, even only for the lines around if it's a few lines in a larger method (of course those shouldn't exist... But they do 🙂) Anything else is just wasting the reviewers time on trying to find out what is actual changes, as well as making history traversal an even bigger pain. `?w=1` helps, but not much when lines gets broken up (as they do most of the time). The noise also makes it easier for problems to slip through, as it's _so_ easy to miss an actual change in what looks like yet another reformat.
You can't even look for highlights, as a moved part will also be highlighted (this is with `?w=1`):
I'll continue, but please don't do that again if you expect people to spend their spare time reviewing it.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148126432
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -63,24 +64,29 @@
import org.slf4j.LoggerFactory;
/**
- * Abstract DataContext for data sources that do not support SQL queries
- * natively.
+ * Abstract DataContext for data sources that do not support SQL queries natively.
*
- * Instead this superclass only requires that a subclass can materialize a
- * single table at a time. Then the query will be executed by post processing
- * the datasets client-side.
+ * Instead this superclass only requires that a subclass can materialize a single table at a time. Then the query will
+ * be executed by post processing the datasets client-side.
*/
public abstract class QueryPostprocessDataContext extends AbstractDataContext implements HasReadTypeConverters {
private static final Logger logger = LoggerFactory.getLogger(QueryPostprocessDataContext.class);
+ public static final String SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS = "metamodel.alias.default.table";
public static final String INFORMATION_SCHEMA_NAME = "information_schema";
- private final Map<Column, TypeConverter<?, ?>> _converters;
+ private final Map<Column, TypeConverter<?, ?>> converters;
+ private final boolean defaultTableEnabled;
public QueryPostprocessDataContext() {
+ this(true);
+ }
+
+ public QueryPostprocessDataContext(boolean defaultTableEnabled) {
--- End diff --
I'd maybe consider another argument and field name, like `singleTableDatastore`. The current name creates an obligation to have a default table, while `singleTableDatastore` would allow tweaking what actually happens in the situation (e.g. if we opt for context flags, like suggested in #128).
I guess a `singleTableDatastore` flag would also allow us to implement optimizations/simplifications later, as we know what to expect. We could also enforce the single table, and maybe save a context developer from surprises and headaches :).
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148126842
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -128,7 +134,7 @@ public DataSet executeQuery(final Query query) {
logger.debug(
"DataContext did not return any count query results. Proceeding with manual counting.");
} else {
- List<Row> data = new ArrayList<Row>(1);
+ final List<Row> data = new ArrayList<Row>(1);
--- End diff --
Unrelated change.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148126897
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -192,8 +198,8 @@ public DataSet executeQuery(final Query query) {
// we can now exclude the select items imposed by the WHERE clause (and
// should, to make the aggregation process faster)
- workSelectItems = CollectionUtils.concat(true, selectItems, groupBySelectItems, havingSelectItems,
- orderBySelectItems);
+ workSelectItems =
+ CollectionUtils.concat(true, selectItems, groupBySelectItems, havingSelectItems, orderBySelectItems);
--- End diff --
Unrelated change.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148438583
--- Diff: core/src/test/java/org/apache/metamodel/QueryPostprocessDataContextTest.java ---
@@ -60,6 +60,49 @@
private final Table table1 = schema.getTableByName(TABLE_CONTRIBUTOR);
private final Table table2 = schema.getTableByName(TABLE_ROLE);
+ public void testSchemaTraversalWithAliasTable() {
+ final MockUpdateableDataContext dc = new MockUpdateableDataContext();
+
+ final Column column = dc.getColumnByQualifiedLabel("foo");
+ assertEquals("table", column.getTable().getName());
+ }
+
--- End diff --
Added those tests now :-)
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148174017
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -63,24 +64,29 @@
import org.slf4j.LoggerFactory;
/**
- * Abstract DataContext for data sources that do not support SQL queries
- * natively.
+ * Abstract DataContext for data sources that do not support SQL queries natively.
*
- * Instead this superclass only requires that a subclass can materialize a
- * single table at a time. Then the query will be executed by post processing
- * the datasets client-side.
+ * Instead this superclass only requires that a subclass can materialize a single table at a time. Then the query will
+ * be executed by post processing the datasets client-side.
*/
public abstract class QueryPostprocessDataContext extends AbstractDataContext implements HasReadTypeConverters {
private static final Logger logger = LoggerFactory.getLogger(QueryPostprocessDataContext.class);
+ public static final String SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS = "metamodel.alias.default.table";
public static final String INFORMATION_SCHEMA_NAME = "information_schema";
- private final Map<Column, TypeConverter<?, ?>> _converters;
+ private final Map<Column, TypeConverter<?, ?>> converters;
+ private final boolean defaultTableEnabled;
public QueryPostprocessDataContext() {
+ this(true);
--- End diff --
I wanted to make sure that I carefully considered every single case of extending the base class. So to begin with I removed the default constructor so that I could check all the compile errors and make sure that every extension of QueryPostprocessDataContext was correctly setting the flag. After that I reintroduced the default constructor to have backwards compatibility.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148367081
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -458,15 +459,18 @@ protected String getDefaultSchemaName() throws MetaModelException {
@Override
protected final Schema getSchemaByNameInternal(final String name) throws MetaModelException {
final String mainSchemaName = getMainSchemaName();
- if (name == null) {
- if (mainSchemaName == null) {
- return getMainSchema();
- }
+ if (name == null && mainSchemaName != null) {
return null;
}
- if (name.equalsIgnoreCase(mainSchemaName)) {
- return getMainSchema();
+ if (name == null || name.equalsIgnoreCase(mainSchemaName)) {
+ final Schema mainSchema = getMainSchema();
+ final boolean createAliasTable = singleTableDatastore
+ && Boolean.parseBoolean(System.getProperty(SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS, "true"));
--- End diff --
Ah, there's my flag! :)
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148126992
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -235,38 +240,28 @@ private boolean isSimpleSelect(SelectClause clause) {
}
/**
- * Executes a simple count query, if possible. This method is provided to
- * allow subclasses to optimize count queries since they are quite common
- * and often a datastore can retrieve the count using some specialized means
- * which is much more performant than counting all records manually.
+ * Executes a simple count query, if possible. This method is provided to allow subclasses to optimize count queries
+ * since they are quite common and often a datastore can retrieve the count using some specialized means which is
+ * much more performant than counting all records manually.
*
- * @param table
- * the table on which the count is requested.
- * @param whereItems
- * a (sometimes empty) list of WHERE items.
- * @param functionApproximationAllowed
- * whether approximation is allowed or not.
+ * @param table the table on which the count is requested.
+ * @param whereItems a (sometimes empty) list of WHERE items.
+ * @param functionApproximationAllowed whether approximation is allowed or not.
--- End diff --
Unrelated change.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148127042
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -235,38 +240,28 @@ private boolean isSimpleSelect(SelectClause clause) {
}
/**
- * Executes a simple count query, if possible. This method is provided to
- * allow subclasses to optimize count queries since they are quite common
- * and often a datastore can retrieve the count using some specialized means
- * which is much more performant than counting all records manually.
+ * Executes a simple count query, if possible. This method is provided to allow subclasses to optimize count queries
+ * since they are quite common and often a datastore can retrieve the count using some specialized means which is
+ * much more performant than counting all records manually.
*
- * @param table
- * the table on which the count is requested.
- * @param whereItems
- * a (sometimes empty) list of WHERE items.
- * @param functionApproximationAllowed
- * whether approximation is allowed or not.
+ * @param table the table on which the count is requested.
+ * @param whereItems a (sometimes empty) list of WHERE items.
+ * @param functionApproximationAllowed whether approximation is allowed or not.
* @return the count of the particular table, or null if not available.
*/
protected Number executeCountQuery(Table table, List<FilterItem> whereItems, boolean functionApproximationAllowed) {
return null;
}
/**
- * Executes a query which obtains a row by primary key (as defined by
- * {@link Column#isPrimaryKey()}). This method is provided to allow
- * subclasses to optimize lookup queries since they are quite common and
- * often a datastore can retrieve the row using some specialized means which
- * is much more performant than scanning all records manually.
+ * Executes a query which obtains a row by primary key (as defined by {@link Column#isPrimaryKey()}). This method is
+ * provided to allow subclasses to optimize lookup queries since they are quite common and often a datastore can
+ * retrieve the row using some specialized means which is much more performant than scanning all records manually.
*
- * @param table
- * the table on which the lookup is requested.
- * @param selectItems
- * the items to select from the lookup query.
- * @param primaryKeyColumn
- * the column that is the primary key
- * @param keyValue
- * the primary key value that is specified in the lookup query.
+ * @param table the table on which the lookup is requested.
+ * @param selectItems the items to select from the lookup query.
+ * @param primaryKeyColumn the column that is the primary key
+ * @param keyValue the primary key value that is specified in the lookup query.
--- End diff --
Unrelated change.
---
[GitHub] metamodel issue #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/165
Are you sure you want me to revert those "unrelated changes"? I only formatted files that I did a lot of rework of and if you ask me the diff is rather easy to read through here in GitHub. 99% of it is just line length in javadoc remarks and it highlights if any wording has changed, too. It just feels overly difficult to make grand changes to a class while not formatting it to a consistent form. No?
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/165
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148127164
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -385,8 +380,8 @@ protected DataSet materializeTable(final Table table, final List<SelectItem> sel
final DataSet dataSet;
if (INFORMATION_SCHEMA_NAME.equals(schemaName)) {
- DataSet informationDataSet = materializeInformationSchemaTable(table,
- buildWorkingSelectItems(selectItems, whereItems));
+ DataSet informationDataSet =
+ materializeInformationSchemaTable(table, buildWorkingSelectItems(selectItems, whereItems));
--- End diff --
Unrelated change.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148123395
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -63,24 +64,29 @@
import org.slf4j.LoggerFactory;
/**
- * Abstract DataContext for data sources that do not support SQL queries
- * natively.
+ * Abstract DataContext for data sources that do not support SQL queries natively.
*
- * Instead this superclass only requires that a subclass can materialize a
- * single table at a time. Then the query will be executed by post processing
- * the datasets client-side.
+ * Instead this superclass only requires that a subclass can materialize a single table at a time. Then the query will
+ * be executed by post processing the datasets client-side.
*/
public abstract class QueryPostprocessDataContext extends AbstractDataContext implements HasReadTypeConverters {
private static final Logger logger = LoggerFactory.getLogger(QueryPostprocessDataContext.class);
+ public static final String SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS = "metamodel.alias.default.table";
public static final String INFORMATION_SCHEMA_NAME = "information_schema";
- private final Map<Column, TypeConverter<?, ?>> _converters;
+ private final Map<Column, TypeConverter<?, ?>> converters;
+ private final boolean defaultTableEnabled;
public QueryPostprocessDataContext() {
+ this(true);
--- End diff --
Pretty minor, but this seems like the a default that adds a lot of code. There's 7 non-test uses of default true, and _32_ non-test uses of default false.
---
[GitHub] metamodel issue #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/165
Oh man, I never got back to this. Will take a full look tonight.
Anyway, yeah might be more significant than I thought.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148127125
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -319,13 +314,13 @@ protected DataSet materializeFromItem(final FromItem fromItem, final List<Select
// materialize left side
final List<SelectItem> leftOn = Arrays.asList(fromItem.getLeftOn());
- fromItemDataSets[0] = materializeFromItem(fromItem.getLeftSide(),
- CollectionUtils.concat(true, selectItems, leftOn));
+ fromItemDataSets[0] =
+ materializeFromItem(fromItem.getLeftSide(), CollectionUtils.concat(true, selectItems, leftOn));
--- End diff --
Unrelated change.
---
[GitHub] metamodel issue #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/165
Any reactions on this approach @tomatophantastico or @LosD? Following our previous discussion ... this should be the way, right?
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148174999
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -63,24 +64,29 @@
import org.slf4j.LoggerFactory;
/**
- * Abstract DataContext for data sources that do not support SQL queries
- * natively.
+ * Abstract DataContext for data sources that do not support SQL queries natively.
*
- * Instead this superclass only requires that a subclass can materialize a
- * single table at a time. Then the query will be executed by post processing
- * the datasets client-side.
+ * Instead this superclass only requires that a subclass can materialize a single table at a time. Then the query will
+ * be executed by post processing the datasets client-side.
*/
public abstract class QueryPostprocessDataContext extends AbstractDataContext implements HasReadTypeConverters {
private static final Logger logger = LoggerFactory.getLogger(QueryPostprocessDataContext.class);
+ public static final String SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS = "metamodel.alias.default.table";
public static final String INFORMATION_SCHEMA_NAME = "information_schema";
- private final Map<Column, TypeConverter<?, ?>> _converters;
+ private final Map<Column, TypeConverter<?, ?>> converters;
+ private final boolean defaultTableEnabled;
public QueryPostprocessDataContext() {
+ this(true);
+ }
+
+ public QueryPostprocessDataContext(boolean defaultTableEnabled) {
--- End diff --
Fixed. Also added some javadoc to the constructor to explain what it means.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148127149
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -319,13 +314,13 @@ protected DataSet materializeFromItem(final FromItem fromItem, final List<Select
// materialize left side
final List<SelectItem> leftOn = Arrays.asList(fromItem.getLeftOn());
- fromItemDataSets[0] = materializeFromItem(fromItem.getLeftSide(),
- CollectionUtils.concat(true, selectItems, leftOn));
+ fromItemDataSets[0] =
+ materializeFromItem(fromItem.getLeftSide(), CollectionUtils.concat(true, selectItems, leftOn));
// materialize right side
final List<SelectItem> rightOn = Arrays.asList(fromItem.getRightOn());
- fromItemDataSets[1] = materializeFromItem(fromItem.getRightSide(),
- CollectionUtils.concat(true, selectItems, rightOn));
+ fromItemDataSets[1] =
+ materializeFromItem(fromItem.getRightSide(), CollectionUtils.concat(true, selectItems, rightOn));
--- End diff --
Unrelated change.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148127021
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -235,38 +240,28 @@ private boolean isSimpleSelect(SelectClause clause) {
}
/**
- * Executes a simple count query, if possible. This method is provided to
- * allow subclasses to optimize count queries since they are quite common
- * and often a datastore can retrieve the count using some specialized means
- * which is much more performant than counting all records manually.
+ * Executes a simple count query, if possible. This method is provided to allow subclasses to optimize count queries
+ * since they are quite common and often a datastore can retrieve the count using some specialized means which is
+ * much more performant than counting all records manually.
*
- * @param table
- * the table on which the count is requested.
- * @param whereItems
- * a (sometimes empty) list of WHERE items.
- * @param functionApproximationAllowed
- * whether approximation is allowed or not.
+ * @param table the table on which the count is requested.
+ * @param whereItems a (sometimes empty) list of WHERE items.
+ * @param functionApproximationAllowed whether approximation is allowed or not.
* @return the count of the particular table, or null if not available.
*/
protected Number executeCountQuery(Table table, List<FilterItem> whereItems, boolean functionApproximationAllowed) {
return null;
}
/**
- * Executes a query which obtains a row by primary key (as defined by
- * {@link Column#isPrimaryKey()}). This method is provided to allow
- * subclasses to optimize lookup queries since they are quite common and
- * often a datastore can retrieve the row using some specialized means which
- * is much more performant than scanning all records manually.
+ * Executes a query which obtains a row by primary key (as defined by {@link Column#isPrimaryKey()}). This method is
+ * provided to allow subclasses to optimize lookup queries since they are quite common and often a datastore can
+ * retrieve the row using some specialized means which is much more performant than scanning all records manually.
--- End diff --
Unrelated change.
---
[GitHub] metamodel issue #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/165
Can't look too much now (on the way to work), but looks good. A bit worried about the size of something that to me is pretty insignificant, though.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148367755
--- Diff: core/src/test/java/org/apache/metamodel/QueryPostprocessDataContextTest.java ---
@@ -60,6 +60,49 @@
private final Table table1 = schema.getTableByName(TABLE_CONTRIBUTOR);
private final Table table2 = schema.getTableByName(TABLE_ROLE);
+ public void testSchemaTraversalWithAliasTable() {
+ final MockUpdateableDataContext dc = new MockUpdateableDataContext();
+
+ final Column column = dc.getColumnByQualifiedLabel("foo");
+ assertEquals("table", column.getTable().getName());
+ }
+
--- End diff --
I'm a bit missing a test to check that the SYSTEM_PROPERTY_CREATE_DEFAULT_TABLE_ALIAS flag actually properly disables the alias table without breaking stuff.
A bit the same with contexts that uses super(false)... Just to make sure that everything is (and keeps being) consistent.
---
[GitHub] metamodel pull request #165: METAMODEL-1165: Add alias default table
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/165#discussion_r148126868
--- Diff: core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java ---
@@ -151,8 +157,8 @@ public DataSet executeQuery(final Query query) {
if (table != null) {
if (isMainSchemaTable(table)) {
final Object operand = whereItem.getOperand();
- final Row row = executePrimaryKeyLookupQuery(table, selectItems, column,
- operand);
+ final Row row =
+ executePrimaryKeyLookupQuery(table, selectItems, column, operand);
--- End diff --
Unrelated change.
---