You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/02/20 08:27:27 UTC
[GitHub] [flink] twalthr opened a new pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
twalthr opened a new pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153
## What is the purpose of the change
This introduces unresolved data types for class-based extraction, name-based resolution, and configuration-based RAW types. Unresolved types behave like regular data types but only after they have been resolved. Using unresolved data types in nested structures leads to further unresolved types. Thus, the usage of unresolved types is type-safe in API.
## Brief change log
- Introduction of `AbstractDataType` as the highest type of resolved and unresolved data types
- Introduction of `DataTypes.of()` for classes and names
- Helper classes and methods for constructing unresolved types type-safe
## Verifying this change
This change added tests and can be verified as follows:
- `DataTypesTest`
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: yes
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? JavaDocs
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#issuecomment-588815440
<!--
Meta data
Hash:bedd2940605a78541fc327086cf9f696ad3fe031 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/149779051 TriggerType:PUSH TriggerID:bedd2940605a78541fc327086cf9f696ad3fe031
-->
## CI report:
* bedd2940605a78541fc327086cf9f696ad3fe031 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/149779051)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] twalthr commented on issue #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
twalthr commented on issue #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#issuecomment-601062112
Thanks for the review @dawidwys. Merging...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] dawidwys commented on a change in pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
dawidwys commented on a change in pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#discussion_r382170607
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/UnresolvedDataType.java
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.types;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.table.catalog.DataTypeFactory;
+import org.apache.flink.table.types.logical.LogicalType;
+
+import javax.annotation.Nullable;
+
+import java.util.function.Function;
+import java.util.function.Supplier;
+
+/**
+ * Partially resolved data type that requires a lookup in a catalog or configuration before creating
+ * the corresponding {@link LogicalType}.
+ *
+ * <p>Users are able to influence the nullability and conversion class even if the actual {@link LogicalType}
+ * is not fully known yet. The information is stored and verified when resolving to {@link DataType}
+ * lazily.
+ */
+@PublicEvolving
+public final class UnresolvedDataType implements AbstractDataType<UnresolvedDataType> {
+
+ private static final String FORMAT = "[%s]"; // indicates that this is an unresolved type
+
+ private final @Nullable Boolean isNullable;
+
+ private final @Nullable Class<?> conversionClass;
+
+ private final Supplier<String> description;
+
+ private final Function<DataTypeFactory, DataType> resolutionFactory;
+
+ private UnresolvedDataType(
+ @Nullable Boolean isNullable,
+ @Nullable Class<?> conversionClass,
+ Supplier<String> description,
Review comment:
Why do we need a supplier here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#issuecomment-588815440
<!--
Meta data
Hash:bedd2940605a78541fc327086cf9f696ad3fe031 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:bedd2940605a78541fc327086cf9f696ad3fe031
-->
## CI report:
* bedd2940605a78541fc327086cf9f696ad3fe031 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#issuecomment-588815440
<!--
Meta data
Hash:bedd2940605a78541fc327086cf9f696ad3fe031 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/149779051 TriggerType:PUSH TriggerID:bedd2940605a78541fc327086cf9f696ad3fe031
-->
## CI report:
* bedd2940605a78541fc327086cf9f696ad3fe031 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/149779051)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] twalthr closed pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
twalthr closed pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#issuecomment-588815440
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "bedd2940605a78541fc327086cf9f696ad3fe031",
"status" : "SUCCESS",
"url" : "https://travis-ci.com/flink-ci/flink/builds/149779051",
"triggerID" : "bedd2940605a78541fc327086cf9f696ad3fe031",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* bedd2940605a78541fc327086cf9f696ad3fe031 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/149779051)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] dawidwys commented on a change in pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
dawidwys commented on a change in pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#discussion_r382176982
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/DataTypeFactory.java
##########
@@ -38,21 +38,29 @@
@PublicEvolving
public interface DataTypeFactory {
+ /**
+ * Creates a type out of an {@link AbstractDataType}.
+ *
+ * <p>If the given type is already a {@link DataType}, the factory will return it unmodified. In
+ * case of {@link UnresolvedDataType}, the factory will resolve it to a {@link DataType}.
+ */
+ <T extends AbstractDataType<T>> DataType createDataType(AbstractDataType<T> abstractDataType);
Review comment:
ditto: `<?>`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] dawidwys commented on a change in pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
dawidwys commented on a change in pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#discussion_r382168576
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/api/DataTypes.java
##########
@@ -73,13 +78,83 @@
* A {@link DataType} can be used to declare input and/or output types of operations. This class
* enumerates all pre-defined data types of the Table & SQL API.
*
+ * <p>For convenience, this class also contains methods for creating {@link UnresolvedDataType}s that
+ * need to be resolved at later stages. This is in particular useful for more complex types that are
+ * expressed as {@link Class} (see {@link #of(Class)}) or types that need to be looked up in a catalog
+ * (see {@link #of(String)}).
+ *
* <p>NOTE: Planners might not support every data type with the desired precision or parameter. Please
* see the planner compatibility and limitations section in the website documentation before using a
* data type.
*/
@PublicEvolving
public final class DataTypes {
+ /**
+ * Creates an unresolved type that will be resolved to a {@link DataType} by analyzing the given
+ * class later.
+ *
+ * <p>During the resolution, Java reflection is used which can be supported by {@link DataTypeHint}
+ * annotations for nested, structured types.
+ *
+ * <p>It will throw an {@link ValidationException} in cases where the reflective extraction needs
+ * more information or simply fails.
+ *
+ * <p>The following examples show how to use and enrich the extraction process:
+ *
+ * <pre>
+ * {@code
+ * // returns INT
+ * of(Integer.class)
+ *
+ * // returns TIMESTAMP(9)
+ * of(java.time.LocalDateTime.class)
+ *
+ * // returns an anonymous, unregistered structured type
+ * // that is deeply integrated into the API compared to opaque RAW types
+ * class User {
+ *
+ * // extract fields automatically
+ * public String name;
+ * public int age;
+ *
+ * // enrich the extraction with precision information
+ * public @DataTypeHint("DECIMAL(10,2)") BigDecimal accountBalance;
+ *
+ * // enrich the extraction with forcing using RAW types
+ * public @DataTypeHint(forceRawPattern = "scala.") Address address;
+ *
+ * // enrich the extraction by specifying defaults
+ * public @DataTypeHint(defaultSecondPrecision = 3) Log log;
+ * }
+ * of(User.class)
+ * }
+ * </pre>
+ *
+ * <p>Note: In most of the cases, the {@link UnresolvedDataType} will be automatically resolved by
+ * the API. At other locations, a {@link DataTypeFactory} is provided.
+ */
+ public static UnresolvedDataType of(Class<?> unresolvedClass) {
Review comment:
nit: How about we move it somewhere down below in this class? It shouldn't be the "to go" method for all types. I think the specific SQL types should be constructed explicitly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] dawidwys commented on a change in pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
dawidwys commented on a change in pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#discussion_r382165231
##########
File path: flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/DataTypeFactoryImpl.java
##########
@@ -61,19 +63,29 @@
}
@Override
- public Optional<DataType> createDataType(String name) {
+ public <T extends AbstractDataType<T>> DataType createDataType(AbstractDataType<T> abstractDataType) {
Review comment:
How about we use a wildcard here? We don't care about the type here.
```
DataType createDataType(AbstractDataType<?> abstractDataType)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] twalthr commented on a change in pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
twalthr commented on a change in pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#discussion_r382457836
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/UnresolvedDataType.java
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.types;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.table.catalog.DataTypeFactory;
+import org.apache.flink.table.types.logical.LogicalType;
+
+import javax.annotation.Nullable;
+
+import java.util.function.Function;
+import java.util.function.Supplier;
+
+/**
+ * Partially resolved data type that requires a lookup in a catalog or configuration before creating
+ * the corresponding {@link LogicalType}.
+ *
+ * <p>Users are able to influence the nullability and conversion class even if the actual {@link LogicalType}
+ * is not fully known yet. The information is stored and verified when resolving to {@link DataType}
+ * lazily.
+ */
+@PublicEvolving
+public final class UnresolvedDataType implements AbstractDataType<UnresolvedDataType> {
+
+ private static final String FORMAT = "[%s]"; // indicates that this is an unresolved type
+
+ private final @Nullable Boolean isNullable;
+
+ private final @Nullable Class<?> conversionClass;
+
+ private final Supplier<String> description;
+
+ private final Function<DataTypeFactory, DataType> resolutionFactory;
+
+ private UnresolvedDataType(
+ @Nullable Boolean isNullable,
+ @Nullable Class<?> conversionClass,
+ Supplier<String> description,
Review comment:
Generating the description can be quite expensive. If you just would like to create a unresolved data type, the string generation should happen lazily.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] twalthr commented on a change in pull request #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
twalthr commented on a change in pull request #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#discussion_r382457836
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/UnresolvedDataType.java
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.types;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.table.catalog.DataTypeFactory;
+import org.apache.flink.table.types.logical.LogicalType;
+
+import javax.annotation.Nullable;
+
+import java.util.function.Function;
+import java.util.function.Supplier;
+
+/**
+ * Partially resolved data type that requires a lookup in a catalog or configuration before creating
+ * the corresponding {@link LogicalType}.
+ *
+ * <p>Users are able to influence the nullability and conversion class even if the actual {@link LogicalType}
+ * is not fully known yet. The information is stored and verified when resolving to {@link DataType}
+ * lazily.
+ */
+@PublicEvolving
+public final class UnresolvedDataType implements AbstractDataType<UnresolvedDataType> {
+
+ private static final String FORMAT = "[%s]"; // indicates that this is an unresolved type
+
+ private final @Nullable Boolean isNullable;
+
+ private final @Nullable Class<?> conversionClass;
+
+ private final Supplier<String> description;
+
+ private final Function<DataTypeFactory, DataType> resolutionFactory;
+
+ private UnresolvedDataType(
+ @Nullable Boolean isNullable,
+ @Nullable Class<?> conversionClass,
+ Supplier<String> description,
Review comment:
Generating the description can be quite expensive. If you just would like to create a resolved data type, the string generation should happen lazily.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #11153:
[FLINK-15958][table-common] Introduce unresolved data types in API
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #11153: [FLINK-15958][table-common] Introduce unresolved data types in API
URL: https://github.com/apache/flink/pull/11153#issuecomment-588760353
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit bedd2940605a78541fc327086cf9f696ad3fe031 (Thu Feb 20 08:30:42 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services