Create kcsb #54

toshetah · 2018-08-14T13:54:25Z

No description provided.

danield137 · 2018-08-14T14:11:35Z

azure-kusto-data/azure/kusto/data/_response.py

                result.append(
                    "Please provide the following data ot Kusto: CRID='{0}' Description:'{1}'".format(
-                        q[self._crid_column], q[self._status_column]
+                        row[self._crid_column], row[self._status_column]


you could do this:

"Please provide the following data ot Kusto: CRID='{0._crid_column}' Description:'{0._status_column}'".format(self)

danield137 · 2018-08-14T14:19:06Z

azure-kusto-data/azure/kusto/data/request.py

+        :param str user_id: AAD user ID.
+        :param str password: Corresponding password of the AAD user.
+        """
+        _ensure_value_is_valid(connection_string)


naming conventions for checks that raise is usually - assert_value_is_valid. assert_non_empty probably better describes the function

danield137 · 2018-08-14T14:30:02Z

azure-kusto-data/azure/kusto/data/request.py

+        :param str user_id: AAD user ID.
+        :param str password: Corresponding password of the AAD user.
+        """
+        _ensure_value_is_valid(connection_string)


should probably assert connection_string in init as this method is more specific for username + password

danield137 · 2018-08-14T14:36:57Z

azure-kusto-data/azure/kusto/data/request.py

+            return self._internal_dict[self.ValidKeywords.authority_id]
+        return None
+
+    @authority_id.setter


isn't this covered by __setitem__ ?

__setitem__ is for x[y] = 1 -- the setter implements x.y = 1 (which is __setattr__).

The body of this setter actually relies on __setitem__ :)

danield137 · 2018-08-14T14:38:01Z

azure-kusto-data/azure/kusto/data/request.py

+
+def _ensure_value_is_valid(value):
+    if not value or not value.strip():
+        raise ValueError


ValueError('Should not be empty')

danield137 · 2018-08-14T14:41:58Z

azure-kusto-data/azure/kusto/data/security.py

+        """Acquire tokens from AAD."""
+        token = self._adal_context.acquire_token(
+            self._kusto_cluster, self._username, self._client_id
+        )
        if token is not None:
            expiration_date = dateutil.parser.parse(token[TokenResponseFields.EXPIRES_ON])


is it in the same timezone?

Yes, https://github.com/AzureAD/azure-activedirectory-library-for-python/blob/1.0.2/adal/oauth2_client.py#L185-L187

zooba · 2018-08-14T16:15:21Z

azure-kusto-data/azure/kusto/data/request.py

+        if connection_string is not None and "=" not in connection_string.partition(";")[0]:
+            connection_string = "Data Source=" + connection_string
+        for kvp_string in connection_string.split(";"):
+            kvp = kvp_string.split("=")


key, _, value = kvp_string.partition("=") is likely better here, as it will better handle strings like "Password=My=Password=Has=Equals=In=It"

zooba · 2018-08-14T16:17:33Z

azure-kusto-data/azure/kusto/data/request.py

-    >>> kusto_client = KustoClient(kusto_cluster, username='your_username', password='your_password')
+    def __setitem__(self, key, value):
+        try:
+            keyword = key if isinstance(key, self.ValidKeywords) else self._Keywords[key.strip()]


For a neat trick, you can also put all the ValidKeywords values in _Keywords and then just unconditionally look it up (depending on how critical the .strip() is - is leading/trailing whitespace something you expect to see? But not case-insensitive?)

Actually the strip is important, and thanks for showing me the case insensitivity, as it is required as well.
What would you recommend in such a case? How to implement a one to many enum, when the keys are case insensitive and should be stripped?

It looks like you really expect an enum, but will also map a str to a member of that enum. So in that case what you have is fine, but I'd flip the logic around and convert str to the enum first:

if isinstance(key, str): # all the keys in _Keywords should be lowercase keyword = self._Keywords.get(key.strip().lower()) if not isinstance(key, self.ValidKeywords): raise ValueError(...) # rest of function

zooba · 2018-08-14T16:18:33Z

azure-kusto-data/azure/kusto/data/request.py

-    KustoClient works with both 2.x and 3.x flavors of Python. All primitive types are supported.
-    KustoClient takes care of ADAL authentication, parsing response and giving you typed result set.
+    @unique
+    class ValidKeywords(Enum):


Any particular reason for this to be nested within this class?

Only a logic reason. Those are the valid keywords to build a Kusto connection string.

One thing to test is that you can put these values into a dict and round-trip it using pickle. Sometimes it can't handle certain nesting, though I think this one will be fine.

zooba · 2018-08-14T16:19:54Z

azure-kusto-data/azure/kusto/data/request.py

+        """
+        if self.ValidKeywords.data_source in self._internal_dict:
+            return self._internal_dict[self.ValidKeywords.data_source]
+        return None


This pattern is the same as return self._internal_dict.get(self.ValidKeywords.data_source) (except the shorter one doesn't have a race condition ;) )

zooba · 2018-08-14T16:21:36Z

azure-kusto-data/azure/kusto/data/request.py

+            return self._internal_dict[self.ValidKeywords.authority_id]
+        return None
+
+    @authority_id.setter


__setitem__ is for x[y] = 1 -- the setter implements x.y = 1 (which is __setattr__).

The body of this setter actually relies on __setitem__ :)

zooba · 2018-08-14T16:23:25Z

azure-kusto-data/tests/test_kusto_client.py

+        data_frame = (
+            client.execute_query("PythonTest", "Deft")
+            .primary_results[0]
+            .to_dataframe(errors="ignore")


Personally I'd indent these two lines again to make it clearer that they're part of the same expression (dots, not commas).

I'd agree with you, but black doesn't :)

Fair enough. Looks like it's being tracked on Black at psf/black#147

Tomer Shetah added 9 commits August 14, 2018 15:18

Create KustoConnectionStringBuilder

26a2274

Rename file

4bf361f

Change more file names

228471a

Final names

429cacb

Rename

19264d8

raname

9234972

edit mock path

40910b3

merge from master

5b2dc97

Black format

ce595e9

toshetah self-assigned this Aug 14, 2018

toshetah requested review from danield137, vladikbr and t-liyari August 14, 2018 13:54

typo

aa3a684

danield137 reviewed Aug 14, 2018

View reviewed changes

Tomer Shetah added 2 commits August 14, 2018 18:27

make primary_results return always a list

911a01b

black formatting

726de3c

zooba reviewed Aug 14, 2018

View reviewed changes

Tomer Shetah added 3 commits August 15, 2018 12:16

Fix steve comments

090a078

typo

8089147

Add support to KCSB with port

237d87d

vladikbr approved these changes Aug 16, 2018

View reviewed changes

toshetah merged commit 493322b into master Aug 16, 2018

toshetah deleted the create_kcsb branch August 16, 2018 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create kcsb #54

Create kcsb #54

toshetah commented Aug 14, 2018

danield137 Aug 14, 2018

danield137 Aug 14, 2018 •

edited

Loading

danield137 Aug 14, 2018 •

edited

Loading

danield137 Aug 14, 2018

zooba Aug 14, 2018

danield137 Aug 14, 2018

danield137 Aug 14, 2018

toshetah Aug 14, 2018 •

edited

Loading

zooba Aug 14, 2018

toshetah Aug 15, 2018

zooba Aug 14, 2018

toshetah Aug 15, 2018

zooba Aug 15, 2018 •

edited

Loading

zooba Aug 14, 2018

toshetah Aug 15, 2018

zooba Aug 15, 2018

zooba Aug 14, 2018

toshetah Aug 15, 2018

zooba Aug 14, 2018

zooba Aug 14, 2018

toshetah Aug 15, 2018

zooba Aug 15, 2018

Create kcsb #54

Create kcsb #54

Conversation

toshetah commented Aug 14, 2018

Choose a reason for hiding this comment

danield137 Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

danield137 Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toshetah Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zooba Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danield137 Aug 14, 2018 •

edited

Loading

danield137 Aug 14, 2018 •

edited

Loading

toshetah Aug 14, 2018 •

edited

Loading

zooba Aug 15, 2018 •

edited

Loading