Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ object FunctionRegistry {
expression[StringLPad]("lpad"),
expression[StringTrimLeft]("ltrim"),
expression[JsonTuple]("json_tuple"),
expression[ParseUrl]("parse_url"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should go before printf

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, Thank you for review. I'll fix this.

expression[FormatString]("printf"),
expression[RegExpExtract]("regexp_extract"),
expression[RegExpReplace]("regexp_replace"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

package org.apache.spark.sql.catalyst.expressions

import java.net.URL
import java.text.{DecimalFormat, DecimalFormatSymbols}
import java.util.regex.Pattern
import java.util.{HashMap, Locale, Map => JMap}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, imports need to be sorted.


import org.apache.spark.sql.catalyst.InternalRow
Expand All @@ -27,6 +29,8 @@ import org.apache.spark.sql.catalyst.util.ArrayData
import org.apache.spark.sql.types._
import org.apache.spark.unsafe.types.{ByteArray, UTF8String}

import scala.util.control.NonFatal

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, we have import ordering. You should move NonFatal like the following. Blank line is needed.

import java.util.Date

import scala.util.control.NonFatal

import org.apache.hadoop._

import org.apache.spark.sql._

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By the way, I'm wondering why this does not cause build errors. Usually, it's caught by build errors.

Anyway, could you run ./dev/scalastyle?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

./dev/scalastyle do give the errors.

I just use IDEA to do the compile job, it seems this script does not be called. I'll manually call it next time. Thanks.


////////////////////////////////////////////////////////////////////////////////////////////////////
// This file defines expressions for string operations.
////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -652,6 +656,125 @@ case class StringRPad(str: Expression, len: Expression, pad: Expression)
override def prettyName: String = "rpad"
}

/**
* Extracts a part from a URL
*/
@ExpressionDescription(
usage = "_FUNC_(url, partToExtract[, key]) - extracts a part from a URL",
extended = """Parts: HOST, PATH, QUERY, REF, PROTOCOL, AUTHORITY, FILE, USERINFO.
Key specifies which query to extract.
Examples:
> SELECT _FUNC_('http://spark.apache.org/path?query=1', 'HOST')\n 'spark.apache.org'
> SELECT _FUNC_('http://spark.apache.org/path?query=1', 'QUERY')\n 'query=1'
> SELECT _FUNC_('http://spark.apache.org/path?query=1', 'QUERY', 'query')\n '1'""")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe,

      > SELECT _FUNC_('http://spark.apache.org/path?query=1', 'HOST')
      'spark.apache.org'
      > SELECT _FUNC_('http://spark.apache.org/path?query=1', 'QUERY')
      'query=1'
      > SELECT _FUNC_('http://spark.apache.org/path?query=1', 'QUERY', 'query')
      '1'""")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK

case class ParseUrl(children: Expression*)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is no vararg in parse url, is there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The parameter key can only be present when partToExtract is QUERY. So there are two parameters call and three parameters call. I have not figured out a better way to accomplish this.

I'll keep on trying to do it better. If there is any advice, that will be very helpful. Thanks again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can just declare a separate ctor that accepts two args

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we use Seq[Expression] here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right. I'll fix this.

extends Expression with ImplicitCastInputTypes with CodegenFallback {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's ok to use CodegenFallback if the codegen is too hard to implement, but we should open a JIRA for the codegen part so we won't forget.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll open a JIRA for the codegen part and keep working on it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here -- i don't think it makes a lot of sense to use ImplicitCastInputTypes here, since we are talking about urls. Why don't we just use ExpectsInputTypes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am trying to make spark's behavior mostly like hive.
As hive does implicit cast for key, eg

hive> select parse_url("http://spark/path?1=v", "QUERY", 1);
v

Should we keep the same in spark?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's OK in this case to not follow. This function is so esoteric that I doubt people will complain. If they do, we can always add the implicit casting later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll use ExpectsInputTypes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually let's just keep it. Might as well since the code is already written.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, I have missed this comment and finished the change...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh well this works


override def nullable = true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

override def nullable: Boolean = true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Public functions need explicit type declaration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, ./dev/scalastyle alse give this error.

override def inputTypes: Seq[DataType] = Seq.fill(children.size)(StringType)
override def dataType: DataType = StringType

@transient private var lastUrlStr: UTF8String = _
@transient private var lastUrl: URL = _
// last key in string, we will update the pattern if key value changed.
@transient private var lastKey: UTF8String = _
// last regex pattern, we cache it for performance concern
@transient private var pattern: Pattern = _

private lazy val stringExprs = children.toArray

@cloud-fan cloud-fan Jul 7, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary... we only have 2 or 3 parameters...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK..

private val HOST = UTF8String.fromString("HOST")
private val PATH = UTF8String.fromString("PATH")
private val QUERY = UTF8String.fromString("QUERY")
private val REF = UTF8String.fromString("REF")
private val PROTOCOL = UTF8String.fromString("PROTOCOL")
private val FILE = UTF8String.fromString("FILE")
private val AUTHORITY = UTF8String.fromString("AUTHORITY")
private val USERINFO = UTF8String.fromString("USERINFO")
private val REGEXPREFIX = "(&|^)"
private val REGEXSUBFIX = "=([^&]*)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we put these constants into an object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Make sense.


override def checkInputDataTypes(): TypeCheckResult = {
if (children.size > 3 || children.size < 2) {
TypeCheckResult.TypeCheckFailure("parse_url function requires two or three arguments")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And, here, let's use $prettyName instead of parse_url. I mean

TypeCheckResult.TypeCheckFailure(s"$prettyName function requires two or three arguments")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, thank you.

} else {
super[ImplicitCastInputTypes].checkInputDataTypes()
}
}

def parseUrlWithoutKey(url: Any, partToExtract: Any): Any = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you make this private?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK

if (url == null || partToExtract == null) {
null
} else {
if (lastUrlStr == null || !url.equals(lastUrlStr)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this optimization mainly for when the url is literal?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. When the url column has many same values.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can follow XPathBoolean to optimize for literal case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thought we judge on the url string, the main purpose is to cache the URL object.
As We must handle the exceptions caused by invalid urls, the approach of XPathBoolean seems not suitable.

try {
lastUrlStr = url.asInstanceOf[UTF8String].clone()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so if all urls are different, we need to clone them everytime?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Em, good point. I'll try to find a better way.

lastUrl = new URL(url.toString)
} catch {
case NonFatal(_) => return null

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should check the possible exceptions for new URL instead of catching NonFatal blindly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As hive returns null for invalid urls, it seems reasonable that spark does the same thing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh this is just code style suggestion: only catch the exception that may be thrown in the try block, new URL only throws some specific kind of exceptions right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it only throws MalformedURLException. But Dongjoon Hyun suggested me to use NonFatal(_) here, as quoted

We can use the following like Hive uses Exception. SecurityException occurs possibly as a RuntimeException.
case NonFatal(_) =>

}
}

if (partToExtract.equals(HOST)) {
UTF8String.fromString(lastUrl.getHost)
} else if (partToExtract.equals(PATH)) {
UTF8String.fromString(lastUrl.getPath)
} else if (partToExtract.equals(QUERY)) {
UTF8String.fromString(lastUrl.getQuery)
} else if (partToExtract.equals(REF)) {
UTF8String.fromString(lastUrl.getRef)
} else if (partToExtract.equals(PROTOCOL)) {
UTF8String.fromString(lastUrl.getProtocol)
} else if (partToExtract.equals(FILE)) {
UTF8String.fromString(lastUrl.getFile)
} else if (partToExtract.equals(AUTHORITY)) {
UTF8String.fromString(lastUrl.getAuthority)
} else if (partToExtract.equals(USERINFO)) {
UTF8String.fromString(lastUrl.getUserInfo)
} else {
null
}
}
}

override def eval(input: InternalRow): Any = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is somewhat convoluted with 4 levels of nesting, I think you can rewrite it this way to make it easier to follow

val evaluated = children.map(_.eval(input).asInstanceOf[UTF8String])
if (evaluated.contains(null)) {
  return null
}

if (evaluated.size == 2) {
  return parseUrlWithoutKey(evaluated(0), evaluated(1))
} else {
  // 3-arg, i.e. QUERY with key
  assert(evaluated.size == 3)
  if (evaluated(1) != QUERY) {
    return null
  }

  val query = parseUrlWithoutKey(evaluated(0), evaluated(1))
  if (query eq null) {
    return null
  }

  if (cachedPattern ne null) {
    return extractValueFromQuery(query, cachedPattern)
  } else {
    return extractValueFromQuery(query, getPattern(evaluated(2)))
  }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Make sense, I'll fix this.

val url = stringExprs(0).eval(input)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

usually we will cast the result of eval to expected type right after we call eval

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Make sense. I am trying to declare vars more explicitly. Thanks.

val partToExtract = stringExprs(1).eval(input)
if (stringExprs.size == 2) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's more clear to put the null handling at the very beginning, e.g.

val evaluated = stringExprs.map(e => e.eval(input).asInstanceOf[UTF8String])
if (evaluated.contains(null)) return null
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, this implementation is very good!

parseUrlWithoutKey(url, partToExtract)
} else { // QUERY with key
if (partToExtract == null || !partToExtract.equals(QUERY)) {
null
} else {
val query = parseUrlWithoutKey(url, partToExtract)
if (query == null) {
null
} else {
val key = stringExprs(2).eval(input)
if (key == null) {
null
} else {
if (!key.equals(lastKey)) {
lastKey = key.asInstanceOf[UTF8String].clone()
val sb = new StringBuilder()
sb.append(REGEXPREFIX).append(key.toString).append(REGEXSUBFIX)
pattern = Pattern.compile(sb.toString())
}

val m = pattern.matcher(query.toString)
if (m.find()) {
UTF8String.fromString(m.group(2))
} else {
null
}
}
}
}
}
}

override def prettyName: String = "parse_url"
}

/**
* Returns the input formatted according do printf-style format strings
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -725,4 +725,45 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkEvaluation(FindInSet(Literal("abf"), Literal("abc,b,ab,c,def")), 0)
checkEvaluation(FindInSet(Literal("ab,"), Literal("abc,b,ab,c,def")), 0)
}

test("ParseUrl") {
def checkParseUrl(expected: String, urlStr: String, partToExtract: String): Unit = {
checkEvaluation(
ParseUrl(Literal.create(urlStr, StringType), Literal.create(partToExtract, StringType)),
expected)
}
def checkParseUrlWithKey(
expected: String, urlStr: String,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one parameter one line please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK

partToExtract: String, key: String): Unit = {
checkEvaluation(
ParseUrl(Literal.create(urlStr, StringType), Literal.create(partToExtract, StringType),
Literal.create(key, StringType)), expected)
}

checkParseUrl("spark.apache.org", "http://spark.apache.org/path?query=1", "HOST")
checkParseUrl("/path", "http://spark.apache.org/path?query=1", "PATH")
checkParseUrl("query=1", "http://spark.apache.org/path?query=1", "QUERY")
checkParseUrl("Ref", "http://spark.apache.org/path?query=1#Ref", "REF")
checkParseUrl("http", "http://spark.apache.org/path?query=1", "PROTOCOL")
checkParseUrl("/path?query=1", "http://spark.apache.org/path?query=1", "FILE")
checkParseUrl("spark.apache.org:8080", "http://spark.apache.org:8080/path?query=1", "AUTHORITY")
checkParseUrl("userinfo", "http://userinfo@spark.apache.org/path?query=1", "USERINFO")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what will happen if there is no userinfo in the url?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then the result is null. I'll add a test case for this.

checkParseUrlWithKey("1", "http://spark.apache.org/path?query=1", "QUERY", "query")

// Null checking
checkParseUrl(null, null, "HOST")
checkParseUrl(null, "http://spark.apache.org/path?query=1", null)
checkParseUrl(null, null, null)
checkParseUrl(null, "test", "HOST")
checkParseUrl(null, "http://spark.apache.org/path?query=1", "NO")
checkParseUrlWithKey(null, "http://spark.apache.org/path?query=1", "HOST", "query")
checkParseUrlWithKey(null, "http://spark.apache.org/path?query=1", "QUERY", "quer")
checkParseUrlWithKey(null, "http://spark.apache.org/path?query=1", "QUERY", null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add exceptional cases by using the following statement?

intercept[AnalysisException] {
  ...
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. Is there any exceptional case?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

e.g. invalid url, invalid part

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I miss the point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As invalid url and invalid part just get null result, I wonder in what circumstance there would throw an exception?

@dongjoon-hyun dongjoon-hyun Jul 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Invalid key? Try this one.

SELECT parse_url('http://spark/?','QUERY', '???')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure how to handle this kind of malformed queries. Hive makes reasonable message.

hive> SELECT parse_url('https://?1=1&?','QUERY', '???');
FAILED: SemanticException [Error 10014]: Line 1:7 Wrong arguments ''???'': 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. Invalid key does this job. I'll fix this.

checkParseUrlWithKey(null, "http://spark.apache.org/path?query=1", "QUERY", "")

// arguments checking
assert(ParseUrl(Literal("1")).checkInputDataTypes().isFailure)
assert(ParseUrl(Literal("1"), Literal("2"), Literal("3"), Literal("4"))
.checkInputDataTypes().isFailure)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,21 @@ class StringFunctionsSuite extends QueryTest with SharedSQLContext {
Row("???hi", "hi???", "h", "h"))
}

test("string parse_url function") {
val df = Seq[String](("http://userinfo@spark.apache.org/path?query=1#Ref"))
.toDF("url")

checkAnswer(
df.selectExpr(
"parse_url(url, 'HOST')", "parse_url(url, 'PATH')",
"parse_url(url, 'QUERY')", "parse_url(url, 'REF')",
"parse_url(url, 'PROTOCOL')", "parse_url(url, 'FILE')",
"parse_url(url, 'AUTHORITY')", "parse_url(url, 'USERINFO')",
"parse_url(url, 'QUERY', 'query')"),
Row("spark.apache.org", "/path", "query=1", "Ref",
"http", "/path?query=1", "userinfo@spark.apache.org", "userinfo", "1"))
}

test("string repeat function") {
val df = Seq(("hi", 2)).toDF("a", "b")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ private[sql] class HiveSessionCatalog(
private val hiveFunctions = Seq(
"hash", "java_method", "histogram_numeric",
"map_keys", "map_values",
"parse_url", "percentile", "percentile_approx", "reflect", "sentences", "stack", "str_to_map",
"percentile", "percentile_approx", "reflect", "sentences", "stack", "str_to_map",
"xpath", "xpath_double", "xpath_float", "xpath_int", "xpath_long",
"xpath_number", "xpath_short", "xpath_string",

Expand Down