[GitHub] flink issue #4710: [FLINK-7446] [table] Change DefinedRowtimeAttribute to wo...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/4710
  
Thanks @wuchong 
I will merge this PR tomorrow.


---

[GitHub] flink issue #4710: [FLINK-7446] [table] Change DefinedRowtimeAttribute to wo...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/4710
  
Looks good to me.

+1


---

[GitHub] flink issue #4710: [FLINK-7446] [table] Change DefinedRowtimeAttribute to wo...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/4710
  
Thanks for the review @twalthr.
I've updated the PR. 

@haohui: This PR preserves the current logic that time attributes are 
exposed as `TIMESTAMP`. I agree that support for time indicators that expose 
themselves as `Long` is desirable. However, this requires quite a few changes 
as we need to extend several functions (incl. `TUMBLE`, `HOP`, etc.) and 
validation logic in some operators (over windows, joins, etc.). So this is not 
a lightweight change and should be done as a separate issue, IMO. 


---

[GitHub] flink issue #4710: [FLINK-7446] [table] Change DefinedRowtimeAttribute to wo...

[twalthr]

Github user twalthr commented on the issue:

https://github.com/apache/flink/pull/4710
  
@haohui We only cast ROWTIME / PROCTIME directly to LONG during runtime, 
the special types are needed during pre-flight phase and validation. We could 
not come up with a better solution that ensures that watermarks stay aligned 
with the rowtime.


---

[GitHub] flink issue #4710: [FLINK-7446] [table] Change DefinedRowtimeAttribute to wo...

[haohui]

Github user haohui commented on the issue:

https://github.com/apache/flink/pull/4710
  
LGTM overall +1.

One question: since we now cast `ROWTIME` / `PROCTIME` directly to `LONG`, 
I wonder, do we want to revisit the decision that creates dedicated types for 
`ROWTIME` / `PROCTIME`?


---