Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Mar 1, 2017

What changes were proposed in this pull request?

When we resolve inline tables in analyzer, we will evaluate the expressions of inline tables.

When it evaluates a TimeZoneAwareExpression expression, an error will happen because the TimeZoneAwareExpression is not associated with timezone yet.

So we need to resolve these TimeZoneAwareExpressions with time zone when resolving inline tables.

How was this patch tested?

Jenkins tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

@viirya
Copy link
Member Author

viirya commented Mar 1, 2017

cc @ueshin

@rxin
Copy link
Contributor

rxin commented Mar 1, 2017

Can we put the test case in a sql file?

@viirya
Copy link
Member Author

viirya commented Mar 1, 2017

Ok. I will put it in inline-table.sql.

@SparkQA
Copy link

SparkQA commented Mar 1, 2017

Test build #73671 has finished for PR 17114 at commit b635db5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class ResolveInlineTables(conf: CatalystConf) extends Rule[LogicalPlan]

@SparkQA
Copy link

SparkQA commented Mar 1, 2017

Test build #73675 has finished for PR 17114 at commit cc6ee95.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Cast(e, targetType)
}
castedExpr match {
case te: TimeZoneAwareExpression => te.withTimeZone(conf.sessionLocalTimeZone).eval()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we traverse the entire tree expression to check for time zone aware expressions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or did ResolveTimeZone already process the input expression? In that case just add the timezone to the cast.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should. ResolveTimeZone is performed after all resolution rules finish.

select * from values ("one", 2.0), ("two", 3.0D) as data(a, b);

-- string to timestamp
select * from values timestamp('1991-12-06 00:00:00.0') as data(a);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put this at the end of the file? That reduces the size of the diff.

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - pending jenkins

@viirya
Copy link
Member Author

viirya commented Mar 3, 2017

Thanks @hvanhovell @rxin

@SparkQA
Copy link

SparkQA commented Mar 3, 2017

Test build #73844 has finished for PR 17114 at commit a981ed5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@hvanhovell
Copy link
Contributor

LGTM - merging to master. Thanks!

@asfgit asfgit closed this in 98bcc18 Mar 3, 2017
ResolveAggregateFunctions ::
TimeWindowing ::
ResolveInlineTables ::
ResolveInlineTables(conf) ::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to fix this bug by re-order analyzer rules?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried before. But the resolution rules will add new timezone-aware expressions, so it still needs the rule to resolve timezone-aware expressions after resolution rules.

@viirya viirya deleted the resolve-timeawareexpr-inline-table branch December 27, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants