You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by superbstreak <gi...@git.apache.org> on 2017/11/15 07:50:36 UTC
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
GitHub user superbstreak opened a pull request:
https://github.com/apache/drill/pull/1037
DRILL-5968: Add support for empty service_host user property
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/superbstreak/drill DRILL-5968-Add-support-for-empty-service_host
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1037.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1037
----
commit 7deed948b05d35ff48287defad4ccefe61f0128f
Author: Rob Wu <ro...@gmail.com>
Date: 2017-11-15T07:27:29Z
DRILL-5968: Add support for empty service_host user property
----
---
[GitHub] drill issue #1037: DRILL-5968: Add support for empty service_host user prope...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on the issue:
https://github.com/apache/drill/pull/1037
Thanks, @parthchandra !
---
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1037
---
[GitHub] drill issue #1037: DRILL-5968: Add support for empty service_host user prope...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/1037
LGTM. (Yes, let's merge this in before 1.12.)
---
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1037#discussion_r151260752
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -28,6 +28,20 @@ class DECLSPEC_DRILL_CLIENT DrillUserProperties{
static const std::map<std::string, uint32_t> USER_PROPERTIES;
DrillUserProperties(){};
+
+ /// @brief Insert or update property value associate with the property key if the value is
+ /// empty or key is undefined. This function is useful for setting default property.
+ ///
+ /// @param in_propName The property name.
+ /// @param in_propValue The property value.
+ void SetDefaultProperty(const std::string& in_propName, const std::string& in_propValue)
--- End diff --
Follow naming convention setDefaultProperty instead of SetDefaultProperty
---
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on a diff in the pull request:
https://github.com/apache/drill/pull/1037#discussion_r151282112
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -28,6 +28,20 @@ class DECLSPEC_DRILL_CLIENT DrillUserProperties{
static const std::map<std::string, uint32_t> USER_PROPERTIES;
DrillUserProperties(){};
+
+ /// @brief Insert or update property value associate with the property key if the value is
+ /// empty or key is undefined. This function is useful for setting default property.
+ ///
+ /// @param in_propName The property name.
+ /// @param in_propValue The property value.
+ void SetDefaultProperty(const std::string& in_propName, const std::string& in_propValue)
+ {
+ // If the element value is empty (newly inserted or user defined), then update.
+ if (m_properties[in_propName].empty())
--- End diff --
If the value doesn't exist, the original implementation with the setProperty(2) would also insert a new key. Not sure if that was intended or not.
---
[GitHub] drill issue #1037: DRILL-5968: Add support for empty service_host user prope...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on the issue:
https://github.com/apache/drill/pull/1037
@parthchandra We should get this in for 1.12?
---
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1037#discussion_r151512282
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -28,6 +28,18 @@ class DECLSPEC_DRILL_CLIENT DrillUserProperties{
static const std::map<std::string, uint32_t> USER_PROPERTIES;
DrillUserProperties(){};
+
+ /// @brief Update the property value associate with the property key if the value is
+ /// empty.
+ ///
+ /// @param in_propName The property name.
+ /// @param in_propValue The property value.
+ void setPropertyIfEmpty(const std::string& in_propName, const std::string& in_propValue){
+ // If the element value is empty, then update.
+ if (isPropSet(in_propName) && m_properties[in_propName].empty()){
--- End diff --
Better check would be
`if ( !isPropSet(in_propName) || m_properties[in_propName].empty() )`
---
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on a diff in the pull request:
https://github.com/apache/drill/pull/1037#discussion_r151282197
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -28,6 +28,20 @@ class DECLSPEC_DRILL_CLIENT DrillUserProperties{
static const std::map<std::string, uint32_t> USER_PROPERTIES;
DrillUserProperties(){};
+
+ /// @brief Insert or update property value associate with the property key if the value is
+ /// empty or key is undefined. This function is useful for setting default property.
+ ///
+ /// @param in_propName The property name.
+ /// @param in_propValue The property value.
+ void SetDefaultProperty(const std::string& in_propName, const std::string& in_propValue)
--- End diff --
gotcha thanks!
---
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1037#discussion_r151260565
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -28,6 +28,20 @@ class DECLSPEC_DRILL_CLIENT DrillUserProperties{
static const std::map<std::string, uint32_t> USER_PROPERTIES;
DrillUserProperties(){};
+
+ /// @brief Insert or update property value associate with the property key if the value is
+ /// empty or key is undefined. This function is useful for setting default property.
+ ///
+ /// @param in_propName The property name.
+ /// @param in_propValue The property value.
+ void SetDefaultProperty(const std::string& in_propName, const std::string& in_propValue)
+ {
+ // If the element value is empty (newly inserted or user defined), then update.
+ if (m_properties[in_propName].empty())
--- End diff --
I would have preferred to use isPropSet to see if the property value is set. Operator[] for a map is intended only to add new values. I believe this might add a new empty element to the map.
---
[GitHub] drill pull request #1037: DRILL-5968: Add support for empty service_host use...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on a diff in the pull request:
https://github.com/apache/drill/pull/1037#discussion_r151515278
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -28,6 +28,18 @@ class DECLSPEC_DRILL_CLIENT DrillUserProperties{
static const std::map<std::string, uint32_t> USER_PROPERTIES;
DrillUserProperties(){};
+
+ /// @brief Update the property value associate with the property key if the value is
+ /// empty.
+ ///
+ /// @param in_propName The property name.
+ /// @param in_propValue The property value.
+ void setPropertyIfEmpty(const std::string& in_propName, const std::string& in_propValue){
+ // If the element value is empty, then update.
+ if (isPropSet(in_propName) && m_properties[in_propName].empty()){
--- End diff --
certainly
---